-
Notifications
You must be signed in to change notification settings - Fork 2
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 Dockerfile and build-and-run-model
workflow for CI model runs
#9
Add Dockerfile and build-and-run-model
workflow for CI model runs
#9
Conversation
82518a4
to
299b40b
Compare
This reverts commit 299b40b.
3a182bd
to
7fff710
Compare
8e5a417
to
a5a64f5
Compare
with: | ||
vcpu: "16.0" | ||
memory: "65536" | ||
role-duration-seconds: 14400 # Worst-case time for a full model run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not correct, but I'm setting it to match the value for the res model for now until we improve model performance and get a better sense of our worst-case times.
@@ -47,7 +47,7 @@ stages: | |||
cache: false | |||
- output/workflow/recipe/model_workflow_recipe.rds: | |||
cache: false | |||
frozen: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this step was erroneously marked as frozen
; we don't cache intermediate outputs so freezing the train
step leads to an error. Billy seemed to think this was reasonable but let me know if I'm off base!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an error. The only step that should really ever be frozen is the ingest step.
@@ -9,6 +9,7 @@ | |||
suppressPackageStartupMessages({ | |||
library(arrow) | |||
library(aws.s3) | |||
library(aws.ec2metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discovered in ccao-data/model-res-avm#26, this package is necessary in order to allow the aws.s3
package to authenticate using credentials in an ECS environment.
This reverts commit 82c3081.
@@ -152,12 +152,12 @@ the 2023 assessment model. | |||
| Percent Population Mobility, Moved From Within Same County in Past Year | acs5 | numeric | | | |||
| Longitude | loc | numeric | | | |||
| Latitude | loc | numeric | | | |||
| Municipality Name | loc | character | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an artifact of removing the var_name_model != "loc_tax_municipality_name"
conditional in README.Rmd
above. I'm not sure why it changes the order of the feature in the table, but it represents the reverse of what happened in #4.
@@ -106,11 +106,9 @@ ccao::vars_dict %>% | |||
) | |||
) %>% | |||
mutate(`Unique to Condo Model` = ifelse( | |||
var_name_model != "loc_tax_municipality_name" & ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need this conditional now that both repos are (temporarily) using loc_cook_municipality_name
.
…ctions"" This reverts commit e801405.
Oops, I forgot to pin to the main branch of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @jeancochrane. Let's get merges going for the actions repo + this, then do test runs of the res + condo pipelines just to make sure everything is working.
@@ -47,7 +47,7 @@ stages: | |||
cache: false | |||
- output/workflow/recipe/model_workflow_recipe.rds: | |||
cache: false | |||
frozen: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an error. The only step that should really ever be frozen is the ingest step.
…o-data/actions""" This reverts commit a852f22.
…h of ccao-data/actions"""" This reverts commit 3f2114e.
This PR adds a new
build-and-run-model
workflow to build, run, and cleanup the model on GitHub Actions. The workflow is configured to run on PRs and workflow dispatch. On pushes to the main branch, the workflow will rebuild the container image for the main branch, but will not run or cleanup the model.This PR mirrors the following PRs in
model-res-avm
:ccao-data/actions
model-res-avm#59Note also that in order to test a full model run, we switch to using the
loc_cook_municipality_name
feature instead ofloc_tax_municipality_name
. This is not actually the correct feature, and we'll need to switch it back in #4, butloc_tax_municipality_name
is not yet present in the training data and properly refreshing the training data is outside the scope of this issue.Closes #5.
Testing
This PR is pinned to the main branch of the
ccao-data/actions
repo, which will not contain the reusable workflow code that this branch needs until ccao-data/actions#1 gets merged. This means that the most recentbuild-and-run-model
run will appear not to be successful. For an example of a successful model run, see this workflow which was run on the corresponding feature branch ofccao-data/actions
containing the reusable workflow code that this repo needs.