Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add design doc for model deployment pipeline #21

Conversation

jeancochrane
Copy link
Contributor

@jeancochrane jeancochrane commented Oct 18, 2023

This PR adds a draft of a design doc proposing we build a new deployment pipeline to allow us to run models on AWS Batch, triggered by a GitHub Actions workflow either manually or via a PR.

The design as currently proposed is simple enough that I don't think it's worth pulling this PR in; rather, I'm opening it for the purposes of discussion, and once the design is approved and we begin work we can make sure that the final design is documented in the README of this repo whenever we do the implementation work.

@jeancochrane jeancochrane linked an issue Oct 18, 2023 that may be closed by this pull request
docs/deployment-design.md Outdated Show resolved Hide resolved
docs/deployment-design.md Outdated Show resolved Hide resolved
@jeancochrane jeancochrane marked this pull request as ready for review October 18, 2023 16:19
docs/deployment-design.md Outdated Show resolved Hide resolved
docs/deployment-design.md Outdated Show resolved Hide resolved
* Use the [`configure-aws-credentials`](https://github.com/aws-actions/configure-aws-credentials) action to authenticate with AWS
* Run Terraform to make sure an AWS Batch job queue and job definition exist for the PR
* Build and push a new Docker image to ECR
* Use the AWS CLI to [submit a job](https://docs.aws.amazon.com/cli/latest/reference/batch/submit-job.html) to the Batch queue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some further complexity added here by DVC. We currently use DVC in two ways:

  1. To do Data Versioning on the input data for the model. This hashes the data in input/ and pushes it to a dedicated S3 bucket using content-addressable storage. This uses the commands dvc push and dvc pull. Note that intermediate steps (model outputs, performance stats, etc.) are NOT currently cached, though they could be.
  2. To run Data Pipelines using dvc repro. This runs the scripts in pipeline/ in the correct order and hashes their output in dvc.lock. This means if a run fails for some reason or upstream dependencies in DVC's DAG change, you can "resume" a run using local intermediate files.

So, some questions stemming from these two use cases:

  1. How will we use DVC within the batch job? This might be as simple as calling dvc pull && dvc repro to run the full pipeline for each job.
  2. How will we use DVC's data versioning for the input data? IMO we should avoid/prohibit building the input data inside a Batch job. But this means we need to pre-build the data, upload it to the S3 cache, then update dvc.lock to reflect the new location, and stop any PRs that don't have the data built.
  3. Should we take advantage of DVC's versioning to cache intermediate objects from each pipeline stage? Caching each stage means that a run with no changes affecting pipeline/01-train.R will completely skip that stage, instead pulling from the cache. This could save us a lot of time but will use more storage and will add complexity.

There's also a separate question about whether or not to expand our usage of DVC. It supports Metrics and Experiments with native git integration. I think it's worth investigating these features to see if they add any value for our use case.

Copy link
Contributor Author

@jeancochrane jeancochrane Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions @dfsnow! Some responses below; I'll also update the doc to reflect these newer ideas.

  1. How will we use DVC within the batch job? This might be as simple as calling dvc pull && dvc repro to run the full pipeline for each job.

That's my thinking as well, and it seems to align with the example workflow that DVC/CML provides showing how to use their tools in GitHub Actions.

  1. How will we use DVC's data versioning for the input data? IMO we should avoid/prohibit building the input data inside a Batch job. But this means we need to pre-build the data, upload it to the S3 cache, then update dvc.lock to reflect the new location, and stop any PRs that don't have the data built.

Based on the Data Versioning documentation, it seems like the recommended workflow is:

  1. Build/edit data locally
  2. Run dvc push to push these changes to remote data storage and update .dvc metadata files in the local repo
  3. Run git add && git commit && git push to update .dvc metadata files in version control
  4. Run dvc pull in any downstream consumers of the changed data, e.g. on a CI run or on a server running the model

I don't love this pattern; in particular, I wish there were an easy way to build/edit the data in a CI workflow, but as far as I can tell that would require configuring the workflow to commit the .dvc metadata to the repo (I've followed this kind of pattern in the past and it works but it's fragile). But it does seem to me that this is how the tools are designed to be used.

This leads me to a related question -- why don't we currently follow step 3 (push .dvc metadata files to version control)? Is it just because we expect all model runs to build their own data?

  1. Should we take advantage of DVC's versioning to cache intermediate objects from each pipeline stage? Caching each stage means that a run with no changes affecting pipeline/01-train.R will completely skip that stage, instead pulling from the cache. This could save us a lot of time but will use more storage and will add complexity.

I think we should definitely try this! Increased storage use seems fine to me since storage is so much cheaper than compute, so I expect caching intermediate outputs would end up saving us money. It doesn't seem like DVC wants to support this use case, but pushing and pulling the cache from S3 seems like it should work in theory. The added complexity does worry me a bit, though, so I think we should think of this as a nice-to-have rather than a core requirement.

There's also a separate question about whether or not to expand our usage of DVC. It supports Metrics and Experiments with native git integration. I think it's worth investigating these features to see if they add any value for our use case.

Metrics and Experiments both look cool, but they seem to just be an alternative implementation of functionality that we've already built into the pipeline, so I think we should only refactor to use them if we can identify a constraint of our implementation that would be improved by switching to DVC's implementation.

One feature that I do think we should test out with an eye toward adoption is CML runners. This would be an abstraction layer that factors out the awscli commands currently described in the doc, and if it works as advertised, it should let us get up and running with this stack much faster than we would otherwise. But I think a lot will depend on how easy it is to debug, so I'll plan to test it before we decide to commit to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I codified some of these thoughts in the design doc in c0ffa5a, but I'll leave this thread open in case we want to continue discussing here 👍🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my thinking as well, and it seems to align with the example workflow that DVC/CML provides showing how to use their tools in GitHub Actions.

Cool. I'm totally down for a simple workflow that:

  1. Spins up a runner on a PR (after manual approval)
  2. Runs dvc pull to fetch manually constructed data
  3. Runs dvc repro to runs the full pipeline

Based on the Data Versioning documentation, it seems like the recommended workflow is:

  1. Build/edit data locally
  2. Run dvc push to push these changes to remote data storage and update .dvc metadata files in the local repo
  3. Run git add && git commit && git push to update .dvc metadata files in version control
  4. Run dvc pull in any downstream consumers of the changed data, e.g. on a CI run or on a server running the model

I don't love this pattern; in particular, I wish there were an easy way to build/edit the data in a CI workflow, but as far as I can tell that would require configuring the workflow to commit the .dvc metadata to the repo (I've followed this kind of pattern in the past and it works but it's fragile). But it does seem to me that this is how the tools are designed to be used.

This leads me to a related question -- why don't we currently follow step 3 (push .dvc metadata files to version control)? Is it just because we expect all model runs to build their own data?

I think this workflow is actually fine. The ingest stage of the pipeline (pipeline/00-ingest.R) takes forever to run, so I'd rather manually run it and push the updated metadata/results than re-build data inside CI.

As for your question re: metadata, we do push the metadata to GitHub. It's in the dvc.lock file. IIRC, DVC has two patterns for metadata storage: storing everything in a single dvc.lock or storing per-stage/output files in .dvc/. I don't remember why we chose the first pattern.

  1. Should we take advantage of DVC's versioning to cache intermediate objects from each pipeline stage? Caching each stage means that a run with no changes affecting pipeline/01-train.R will completely skip that stage, instead pulling from the cache. This could save us a lot of time but will use more storage and will add complexity.

I think we should definitely try this! Increased storage use seems fine to me since storage is so much cheaper than compute, so I expect caching intermediate outputs would end up saving us money. It doesn't seem like DVC wants to support this use case, but pushing and pulling the cache from S3 seems like it should work in theory. The added complexity does worry me a bit, though, so I think we should think of this as a nice-to-have rather than a core requirement.

I actually think this wouldn't work because it would require us to push the updated dvc.lock back to GitHub in order to keep track of the cache state (or store it elsewhere). IMO, let's start with the simplest possible setup (as I described above).

There's also a separate question about whether or not to expand our usage of DVC. It supports Metrics and Experiments with native git integration. I think it's worth investigating these features to see if they add any value for our use case.

Metrics and Experiments both look cool, but they seem to just be an alternative implementation of functionality that we've already built into the pipeline, so I think we should only refactor to use them if we can identify a constraint of our implementation that would be improved by switching to DVC's implementation.

One feature that I do think we should test out with an eye toward adoption is CML runners. This would be an abstraction layer that factors out the awscli commands currently described in the doc, and if it works as advertised, it should let us get up and running with this stack much faster than we would otherwise. But I think a lot will depend on how easy it is to debug, so I'll plan to test it before we decide to commit to it.

I think this is correct. Let's ignore experiments and metrics for now and test out the CML runners, since I think they basically do exactly what we need.

Another note for the design doc: this year we want to use the following pattern:

  • Use an in-repo Quarto doc to analyze/diagnose/display single model performance. This will be created for each run and will be sent as a link along with the SNS notification at the end of a run.
  • Use Tableau for cross-model comparison, using the same dashboards as previous years.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this all makes sense @dfsnow! I made some modifications in c0ffa5a to remove the caching task and clarify the output task. A couple follow-up questions:

  1. Is reports/model_qc/model_qc.qmd the Quarto doc that we want to generate for model performance, or do we need to create a new one?
  2. Does any work need to be done on the Tableau cross-model dashboards, or can we expect that those will hook into the pipeline in the same way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeancochrane

  1. My plan is to make a new one, but it will likely include outputs from that doc.
  2. Nope. As long as the main outputs to the Athena model.* tables don't change, Tableau doesn't need to change either.

docs/deployment-design.md Show resolved Hide resolved
docs/deployment-design.md Outdated Show resolved Hide resolved
@jeancochrane
Copy link
Contributor Author

The changes described in this doc are now codified in this issue, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft design doc for 2024 modeling infrastructure
2 participants