-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor and simplify pipeline #66
Conversation
Type: Project | ||
Description: Residential valuation model of the Cook County Assessor's Office | ||
Depends: | ||
arrow, | ||
assessr, | ||
aws.s3, | ||
aws.ec2metadata, | ||
butcher, | ||
ccao, | ||
conflicted, | ||
dplyr, | ||
furrr, | ||
git2r, | ||
glue, | ||
hardhat, | ||
here, | ||
knitr, | ||
parsnip, | ||
purrr, | ||
lightgbm, | ||
lightsnip, | ||
lubridate, | ||
paws.analytics, | ||
paws.application.integration, | ||
recipes, | ||
rlang, | ||
rsample, | ||
stringr, | ||
tictoc, | ||
tidyr, | ||
tune, | ||
workflows, | ||
yaml, | ||
yardstick | ||
Remotes: | ||
ccao-data/assessr, | ||
ccao-data/ccao, | ||
ccao-data/lightsnip |
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.
Since I consolidated all the library loading to R/setup.R
and we explicitly track the dev dependencies via profiles, I figured it can't hurt to also make our core dependency tracking explicit.
DBI, | ||
igraph, | ||
lubridate, | ||
markdown, |
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.
Required for building README.Rmd
.
@@ -2,36 +2,22 @@ | |||
# 1. Setup --------------------------------------------------------------------- | |||
#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - | |||
|
|||
# NOTE: See DESCRIPTION for library dependencies and R/setup.R for |
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 added this note at the beginning of each pipeline stage because people are used to seeing calls to library()
, so we should direct them to the correct place.
|
||
# Load R libraries | ||
# Load additional dev R libraries (see README#managing-r-dependencies) |
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.
Here we load the dev dependencies from the separate profile in addition to the main set of dependencies.
# Load libraries and scripts | ||
options(tidymodels.dark = TRUE) | ||
suppressPackageStartupMessages({ | ||
library(arrow) | ||
library(butcher) | ||
library(ccao) | ||
library(dplyr) | ||
library(here) | ||
library(lightgbm) | ||
library(lightsnip) | ||
library(tictoc) | ||
library(tidymodels) | ||
library(vctrs) | ||
library(yaml) | ||
}) | ||
|
||
# Load helpers and recipes from files | ||
walk(list.files("R/", "\\.R$", full.names = TRUE), source) | ||
|
||
# Initialize a dictionary of file paths. See misc/file_dict.csv for details | ||
paths <- model_file_dict() | ||
|
||
# Load the parameters file containing the run settings | ||
params <- read_yaml("params.yaml") | ||
|
||
# Override the default CV toggle from params.yaml. This is useful for manually | ||
# running "limited" runs without CV or assessment data (also used for GitHub CI) | ||
cv_enable <- as.logical( | ||
Sys.getenv("CV_ENABLE_OVERRIDE", unset = params$toggle$cv_enable) | ||
) | ||
|
||
# Get the number of available physical cores to use for lightgbm multi-threading | ||
# Lightgbm docs recommend using only real cores, not logical | ||
# https://lightgbm.readthedocs.io/en/latest/Parameters.html#num_threads | ||
num_threads <- parallel::detectCores(logical = FALSE) | ||
|
||
# Set the overall stage seed | ||
set.seed(params$model$seed) |
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.
The big setup block from all the pipeline stages is for the most part entirely contained in R/setup.R
.
"package.dependency.fields": [ | ||
"Imports", | ||
"Depends", | ||
"LinkingTo" | ||
], |
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 required or renv will try to snapshot both the profile-specific key in the DESCRIPTION
file and everything in the Depends:
key.
@@ -12,7 +12,7 @@ | |||
"ppm.enabled": true, | |||
"ppm.ignored.urls": [], | |||
"r.version": [], | |||
"snapshot.type": "implicit", | |||
"snapshot.type": "explicit", |
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.
Switch to explicit tracking in the DESCRIPTION
file.
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 consolidated all the redundant setup sections of each stage into this single script. The only thing we really lose is information about which dependencies are used per stage, but this is much cleaner and just uses the Depends:
key in the DESCRIPTION
.
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.
Moved this to .github/scripts/
since it's unlikely to be used by any pipeline users and is specific to GitHub. Also, it gets called at the start of every script if it's in R/
by:
purrrr::walk(list.files("R/", "\\.R$", full.names = TRUE), source)
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.
Managed to cut all the additional tidymodels
dependencies that we weren't using by replacing the tidymodels
meta package with explicit dependencies only for the libraries we're using.
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.
Nice cleanup!
# Load helpers for model dictionary, data loading, and other misc functions | ||
source(here::here("R", "helpers.R")) |
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.
[Question, non-blocking] Given that we seem to walk the R
directory to load this script in each pipeline stage, do we actually need to source R/helpers.R
here manually, or can we count on the walk()
call doing so?
model-res-avm/pipeline/00-ingest.R
Lines 12 to 13 in fe5a790
# Load libraries, helpers, and recipes from files | |
purrr::walk(list.files("R/", "\\.R$", full.names = TRUE), source) |
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 need to in order to load model_file_dict()
within the setup script, else loading it is dependent on the walk order.
"Train" = "01", "Assess" = "02", | ||
"Evaluate" = "03", "Interpret" = "04", | ||
"Finalize" = "05" | ||
"Train" = "01", "Assess" = "02", "Evaluate" = "03", |
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.
finally
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.
The pipeline is getting pretty damn slick
This PR lays the groundwork for further 2024 modeling work by:
DESCRIPTION
file.github/scripts/
for clarityDependencies
Unfortunately, I've think we've hit the limit on cleaning up the dependency tree. The tidymodels sub-packages are extremely dependency heavy, so unless we want to switch away from tidymodels we're stuck with ~130 deps. Here's the dependency tree for
workflows
alone:Click here for tune dependency graph
Related to #33. Closes #53.