Generate and publish a quarto doc with performance results on each model run #62

jeancochrane · 2023-11-24T17:36:21Z

This PR updates the finalize step of the model pipeline to render a stub Quarto doc intended to record performance results for the model. After rendering the doc, the model pipeline then uploads it to S3 and adds its console link to the body of the model SNS notification.

In order to add this step, we also need some way to manage the dependencies for rendering the Quarto doc. This PR proposes we do so by adding a new reporting renv profile and a supplemental lockfile in reports/renv.lock to go along with it. Detailed instructions for maintaining this approach are provided in edits to the model README.

Successful model run here. While the link to the performance report will not be sent via SNS notification until #63 is fixed in #64, you can download the performance report at for inspection at s3://ccao-model-results-us-east-1/report/year=2023/report_type=performance/2023-12-01-silly-tayun.html.

Connects #24.

…arto-doc-with-performance-results-on-each-model-run

Dockerfile

DESCRIPTION

R/helpers.R

README.Rmd

jeancochrane · 2023-11-28T17:29:10Z

README.md

-| Sale After COVID-19                                                     | Time           | logical     |                                                                              | Indicator for whether sale occurred after COVID-19 was widely publicized (around March 15, 2020)                                                                                                                                                                                                   |
+model as of 2023-11-24.
+
+| Feature Name                                                            | Category       | Type        | Possible Values                                                              | Notes                                                                                                                                                                                                                                                                                                                                                                                                                                                                |


I believe that the diff in this table represents the new feature docs that we have added to dbt since we last updated the README.

issue (blocking): This is way too much info for this table. Can we edit the README script to only take the description data up to and excluding the first period of each description?

Good call, done in 25c8d91 with the README being reknit in 18cfbce!

jeancochrane · 2023-11-28T17:30:39Z

pipeline/05-finalize.R

  library(here)
  library(lubridate)
  library(paws.application.integration)
  library(purrr)
+  library(quarto)


Per my note above, one way we could make this smoother in the case where the user doesn't choose to install reporting dependencies would be to check if this library is installed or not and skip the generation/upload steps below if so.

I think instead we should just assume that they will never need to run the finalize stage and instead guide them toward just running the report directly.

Per my comment above, finalize should now be runnable by outside users, but we've also added a tryCatch block around report generation that should fail more gracefully if report dependencies are not installed.

jeancochrane · 2023-11-28T17:33:12Z

pipeline/05-finalize.R

+  # Upload performance report
+  aws.s3::put_object(
+    paths$output$report$local,
+    paths$output$report$s3
+  )


Sample report here. Accessing the report itself is a bit annoying; since all objects in ccao-model-results-us-east-1 must be private, we can't simply click the Object URL link to download the report, and instead we have to run aws s3 cp to get a copy. If we're willing to accept the slight increase in security risk, it might make sense to add an access policy specifying that any *.html object under the report/ prefix should be public.

suggestion: Could we generate a pre-signed S3 URL to include with the output with a time expiration? If that's easy to do then it seems like the perfect fit for this use case.

This is a great idea! Unfortunately I couldn't get it to work in ~2 hours of work so I abandoned the idea. It works fine via the aws s3 presign CLI, but something about paws.storage::s3()$generate_presigned_url() doesn't work properly in either RStudio or the ECS task container environment and was throwing me various opaque access denied errors in spite of everything meeting spec. I think we should punt on this unless/until the current workflow becomes bothersome; hopefully the extra step to navigate to the console page and click the Download button won't be too bad.

jeancochrane · 2023-11-28T17:35:26Z

pipeline/05-finalize.R

    # Publish to SNS
    pipeline_sns$publish(
      Subject = paste("Model Run Complete:", run_id),
      Message = paste0(
        "Model run: ", run_id, " complete\n",
        "Finished in: ", pipeline_sns_total_time, "\n\n",
+        "Report link: ", report_url, "\n\n",


In the process of making this change, I realized that I haven't been getting any of these SNS emails recently. Have you? If not, I'll file an issue to investigate why we're missing them; otherwise it's probably just an issue with my subscription or my Outlook config.

No I haven't! Good catch, and very weird. There could be something in the params file preventing notification. Or this step could just be misconfigured. Either way, let's make an issue for it.

Sounds good, issue here: #63

dfsnow

Nice job @jeancochrane! I think this is a good trade off between complexity and being able to isolate the model dependencies (which I'm going to do more of shortly).

If it makes sense to you, let's add a README section on how to run the reporting independent of the finalize stage.

README.Rmd

dfsnow · 2023-11-29T15:55:16Z

README.Rmd

+
+The process for updating **model report dependencies** is more complex, since it requires the use of a separate `reporting` profile:
+
+1. Run `Sys.setenv(RENV_PROFILE = "reporting")` to set the renv profile to `reporting`


nitpick: Shouldn't renv::activate() be used here?

This is a bit tricky -- according to the docs, calling renv::activate() with a profile argument will automatically set that profile to be the default:

This creates a profile called "dev", and sets it as the default for the project, so that newly-launched R sessions will operate using the "dev" profile. [...] Alternatively, if you want to activate a particular profile for an R session without setting it as the default for new R sessions, you can use [Sys.setenv(RENV_PROFILE = "dev")].

I think we don't want the reporting profile to be the default, since I expect we want it to be easier to update model dependencies than reporting dependencies, but maybe I'm wrong? In any case, if you agree that this is the right approach, I'll add a quick note to the docs clarifying why we recommend Sys.setenv() instead of renv::activate().

I think we do actually want to use renv::activate(), since just setting the env var won't change the profile without a restart if I understand correctly. We'd then just call it again to switch back to the default profile.

Gotcha, I switched up the docs to recommend renv::activate() in 5da06da!

README.Rmd

dfsnow · 2023-11-29T15:58:39Z

pipeline/05-finalize.R

  library(here)
  library(lubridate)
  library(paws.application.integration)
  library(purrr)
+  library(quarto)


I think instead we should just assume that they will never need to run the finalize stage and instead guide them toward just running the report directly.

dfsnow · 2023-11-29T16:02:40Z

pipeline/05-finalize.R

+here("reports/performance/performance.qmd") %>%
+  quarto_render(
+    execute_params = list(
+      run_id = run_id,
+      year = params$assessment$year
+    )
+  )


thought: One big issue I foresee with this is that any failure in the Quarto doc is also going to kill the entire pipeline. I foresee a model run that's been running for 50 hours being killed by a misplaced comma in the doc :(

suggestion (blocking): I recommend that we add some kind of error handling here to prevent the doc from killing the pipeline. The result of the error handling should be included in the SNS notification.

dfsnow · 2023-11-29T16:05:09Z

pipeline/05-finalize.R

+  # Upload performance report
+  aws.s3::put_object(
+    paths$output$report$local,
+    paths$output$report$s3
+  )


suggestion: Could we generate a pre-signed S3 URL to include with the output with a time expiration? If that's easy to do then it seems like the perfect fit for this use case.

dfsnow · 2023-11-29T16:06:21Z

pipeline/05-finalize.R

    # Publish to SNS
    pipeline_sns$publish(
      Subject = paste("Model Run Complete:", run_id),
      Message = paste0(
        "Model run: ", run_id, " complete\n",
        "Finished in: ", pipeline_sns_total_time, "\n\n",
+        "Report link: ", report_url, "\n\n",


No I haven't! Good catch, and very weird. There could be something in the params file preventing notification. Or this step could just be misconfigured. Either way, let's make an issue for it.

reports/performance/performance.qmd

jeancochrane · 2023-12-01T17:49:59Z

misc/file_dict.csv

+output,shap,4,interpret,ccao-model-results-us-east-1,output/shap/model_shap.parquet,shap/,shap,card,"year, run_id, township_code, meta_pin, meta_card_num",No,Yes,SHAP values for each feature for each card in the assessment data,NOTE: Each run adds new partitions to S3 which must be added via a Glue crawler
+output,feature_importance,4,interpret,ccao-model-results-us-east-1,output/feature_importance/model_feature_importance.parquet,feature_importance/year={year}/{run_id}.parquet,feature_importance,predictor,"year, run_id, model_predictor_all_name",No,Yes,"Feature importance values (gain, cover, and frequency) for the run",
+output,report,5,finalize,ccao-model-results-us-east-1,reports/performance.html,report/year={year}/report_type=performance/{run_id}.html,,model run,,No,Yes,Rendered Quarto doc with model performance statistics,


In spite of the horrendous diff for this file, the addition of this line should be the only change.

Is this just a line ending change? Can we make sure it's LF like the rest of the files in this repo (or at least they should be).

I'm not totally sure what the source of the diff is, cat -e misc/file_dict.csv shows each line ending with a Unix line ending $. Enabling Hide whitespace in the GitHub UI seems to filter the diff as expected. Let me know if there's any more debugging you'd like me to do, I'm not particularly well-versed in line endings!

jeancochrane · 2023-12-01T17:50:41Z

pipeline/06-upload.R

+#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+# 1. Setup ---------------------------------------------------------------------
+#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -


The code in this file was extracted from 05-finalize.R with very few changes. Since they aren't visible in the diff, I'll call them out in comments below.

jeancochrane · 2023-12-01T17:51:07Z

pipeline/06-upload.R

+suppressPackageStartupMessages({
+  library(arrow)
+  library(aws.s3)
+  library(aws.ec2metadata)
+  library(dplyr)
+  library(glue)
+  library(here)
+  library(knitr)
+  library(lubridate)
+  library(paws.analytics)
+  library(paws.application.integration)
+  library(tidyr)
+  library(yaml)
+})


Dependencies have been cleaned up here so that we only import what we need for this pipeline stage.

jeancochrane · 2023-12-01T17:51:31Z

pipeline/06-upload.R

+# Load various overridden parameters as defined in the `finalize` step
+metadata <- read_parquet(paths$output$metadata$local)
+cv_enable <- metadata$cv_enable
+shap_enable <- metadata$shap_enable
+run_id <- metadata$run_id
+run_type <- metadata$run_type


Since these attributes are generated in 05-finalize.R, we have to load them from the output of that stage now.

jeancochrane · 2023-12-01T17:51:49Z

pipeline/06-upload.R

+  # Upload performance report
+  aws.s3::put_object(
+    paths$output$report$local,
+    paths$output$report$s3
+  )
+}


The performance report upload is new as of this PR.

jeancochrane · 2023-12-01T17:52:53Z

pipeline/06-upload.R

+    # Get a link to the uploaded Quarto report
+    report_path_parts <- strsplit(paths$output$report$s3[1], "/")[[1]]
+    report_bucket <- report_path_parts[3]
+    report_path <- report_path_parts[4:length(report_path_parts)] %>%
+      paste(collapse = "/")
+    # Use direct link to the console instead of to the object so that we don't
+    # have to bother with signed URLs
+    report_url <- paste0(
+      "https://s3.console.aws.amazon.com/s3/object/",
+      "{report_bucket}/{report_path}?region=us-east-1&tab=overview"
+    ) %>%
+      glue::glue()
+
+    # Publish to SNS
+    pipeline_sns$publish(
+      Subject = paste("Model Run Complete:", run_id),
+      Message = paste0(
+        "Model run: ", run_id, " complete\n",
+        "Finished in: ", pipeline_sns_total_time, "\n\n",
+        "Report link: ", report_url, "\n\n",
+        pipeline_sns_results
+      ),
+      TopicArn = Sys.getenv("AWS_SNS_ARN_MODEL_STATUS")
+    )
+  }


Adding a report link to the SNS notification is new as of this PR. Note that this hasn't been tested yet because SNS notifications won't be sent until #63 is fixed by #64.

jeancochrane · 2023-12-01T17:53:34Z

@dfsnow This is ready for another look!

dfsnow · 2023-12-01T18:12:50Z

README.Rmd

+
+The process for updating **model report dependencies** is more complex, since it requires the use of a separate `reporting` profile:
+
+1. Run `Sys.setenv(RENV_PROFILE = "reporting")` to set the renv profile to `reporting`


I think we do actually want to use renv::activate(), since just setting the env var won't change the profile without a restart if I understand correctly. We'd then just call it again to switch back to the default profile.

dfsnow · 2023-12-01T18:13:54Z

README.Rmd

+## Updating R dependencies
+
+There are two lockfiles that we use with renv to manage R dependencies:
+
+1. **`renv.lock`** is the canonical list of dependencies that are used by the **core model pipeline**. Any dependencies that are required to run the model itself should be defined in this lockfile.
+2. **`renv/profiles/reporting/renv.lock`** is the canonical list of dependencies that are used to **generate a model performance report** in the `finalize` step of the pipeline. Any dependencies that are required to generate that report or others like it should be defined in this lockfile.
+
+Our goal in maintaining multiple lockfiles is to keep the list of dependencies that are required to run the model as short as possibile. This choice adds overhead to the process of updating R dependencies, but incurs the benefit of a more maintainable model over the long term.
+
+The process for **updating core model pipeline dependencies** is straightforward: Running `renv::install("<dependency_name>")` and `renv::snapshot()` will ensure that the dependency gets added or updated in `renv.lock`, as long is it is imported somewhere in the model pipeline via a `library(<dependency_name>)` call.
+
+The process for updating **model report dependencies** is more complex, since it requires the use of a separate `reporting` profile:
+
+1. Run `Sys.setenv(RENV_PROFILE = "reporting")` to set the renv profile to `reporting`


suggestion (blocking): Let's generalize these instructions a bit since I'm going to add one more lock file for the ingest and export dependencies.

Sure thing, done in 5da06da.

dfsnow · 2023-12-01T18:16:32Z

README.md

-| Sale After COVID-19                                                     | Time           | logical     |                                                                              | Indicator for whether sale occurred after COVID-19 was widely publicized (around March 15, 2020)                                                                                                                                                                                                   |
+model as of 2023-11-24.
+
+| Feature Name                                                            | Category       | Type        | Possible Values                                                              | Notes                                                                                                                                                                                                                                                                                                                                                                                                                                                                |


issue (blocking): This is way too much info for this table. Can we edit the README script to only take the description data up to and excluding the first period of each description?

dfsnow · 2023-12-01T18:17:00Z

README.md

@@ -708,7 +717,7 @@ the following major changes to the residential modeling codebase:
  process was moved to [pipeline/00-ingest.R](pipeline/00-ingest.R),
  while the process to [finalize model
  values](https://gitlab.com/ccao-data-science---modeling/processes/finalize_model_values)
-  was moved to [pipeline/06-export.R](pipeline/06-export.R).
+  was moved to [pipeline/06-export.R](pipeline/07-export.R).


Suggested change

was moved to [pipeline/06-export.R](pipeline/07-export.R).

was moved to [pipeline/07-export.R](pipeline/07-export.R).

Done in 7458bab.

dfsnow · 2023-12-01T18:18:06Z

README.md

+    workflow to delete test run artifacts in S3 using GitHub Actions.
+  - Updated [pipeline/05-finalize](pipeline/05-finalize.R) step to
+    render a performance report using Quarto and factored S3/SNS
+    operations out into \[pipeline/06-upload.R\].


Suggested change

operations out into \[pipeline/06-upload.R\].

operations out into [pipeline/06-upload.R](pipeline/06-upload.R).

Done in 7458bab.

dfsnow · 2023-12-01T18:19:39Z

misc/file_dict.csv

+output,shap,4,interpret,ccao-model-results-us-east-1,output/shap/model_shap.parquet,shap/,shap,card,"year, run_id, township_code, meta_pin, meta_card_num",No,Yes,SHAP values for each feature for each card in the assessment data,NOTE: Each run adds new partitions to S3 which must be added via a Glue crawler
+output,feature_importance,4,interpret,ccao-model-results-us-east-1,output/feature_importance/model_feature_importance.parquet,feature_importance/year={year}/{run_id}.parquet,feature_importance,predictor,"year, run_id, model_predictor_all_name",No,Yes,"Feature importance values (gain, cover, and frequency) for the run",
+output,report,5,finalize,ccao-model-results-us-east-1,reports/performance.html,report/year={year}/report_type=performance/{run_id}.html,,model run,,No,Yes,Rendered Quarto doc with model performance statistics,


Is this just a line ending change? Can we make sure it's LF like the rest of the files in this repo (or at least they should be).

dfsnow · 2023-12-01T18:20:11Z

pipeline/05-finalize.R

+# defined separately, so this script can't be sure that it is error-free, and
+#


Suggested change

# defined separately, so this script can't be sure that it is error-free, and

#

# defined separately, so this script can't be sure that it is error-free

Fixed in 31dc99d.

dfsnow · 2023-12-01T18:23:26Z

pipeline/05-finalize.R

issue (blocking): Let's add the same tictoc timing code from the earlier stages to this one, since I'm anticipating that the report will take awhile to generate.

Good call, done in 31dc99d!

jeancochrane · 2023-12-01T19:50:15Z

Ready for another look @dfsnow!

dfsnow

Looks good @jeancochrane! Let's merge it.

…arto-doc-with-performance-results-on-each-model-run

jeancochrane added 9 commits November 21, 2023 19:58

Generate and upload model performance report in finalize pipeline step

b16a3e6

Merge branch 'master' into 24-infra-updates-generate-and-publish-a-qu…

186fe1e

…arto-doc-with-performance-results-on-each-model-run

Include .html files in model_get_s3_artifacts_for_run

331b241

Refactor repo to support reports/renv.lock lockfile

0144f28

Remove unnecessary changes to renv/activate.R

af287d2

Fix missing column in performance report row of misc/file_dict.csv

b616399

Update README with instructions on updating R dependencies

25f2850

Add quarto to DESCRIPTION dependencies

9f1c1ca

Move reports/renv.lock -> renv/profiles/reporting/renv.lock

aedd58b

jeancochrane linked an issue Nov 24, 2023 that may be closed by this pull request

[Infra updates] Generate and publish a Quarto doc with performance results on each model run #24

Closed

jeancochrane had a problem deploying to deploy November 24, 2023 17:42 — with GitHub Actions Failure

Properly style R/helpers.R

7ae4f6b

jeancochrane had a problem deploying to deploy November 24, 2023 17:44 — with GitHub Actions Failure

jeancochrane force-pushed the 24-infra-updates-generate-and-publish-a-quarto-doc-with-performance-results-on-each-model-run branch from 8af5c2b to 9a600ab Compare November 24, 2023 21:11

jeancochrane had a problem deploying to deploy November 24, 2023 21:19 — with GitHub Actions Failure

Install Quarto in Dockerfile

fd6538b

jeancochrane force-pushed the 24-infra-updates-generate-and-publish-a-quarto-doc-with-performance-results-on-each-model-run branch from 9a600ab to fd6538b Compare November 27, 2023 20:28

jeancochrane had a problem deploying to deploy November 27, 2023 20:49 — with GitHub Actions Failure

Use the correct path to performance.qmd in 05-finalize.R step

7b39d2f

jeancochrane temporarily deployed to deploy November 27, 2023 23:28 — with GitHub Actions Inactive

jeancochrane commented Nov 28, 2023

View reviewed changes

jeancochrane marked this pull request as ready for review November 28, 2023 17:37

jeancochrane requested review from dfsnow and wrridgeway as code owners November 28, 2023 17:37

dfsnow reviewed Nov 29, 2023

View reviewed changes

reports/performance/performance.qmd Outdated Show resolved Hide resolved

Move performance.qmd to the top level of the reports/ subdir

65948dd

jeancochrane had a problem deploying to deploy November 29, 2023 22:31 — with GitHub Actions Failure

jeancochrane force-pushed the 24-infra-updates-generate-and-publish-a-quarto-doc-with-performance-results-on-each-model-run branch 2 times, most recently from b663934 to ffb017d Compare November 29, 2023 22:36

jeancochrane and others added 2 commits November 30, 2023 21:34

Fix typo in README.Rmd and regenerate README

6772f45

Fix mixed up deps/outputs between finalize and upload stages

ec1c35f

jeancochrane had a problem deploying to deploy November 30, 2023 21:37 — with GitHub Actions Failure

Add missing run_id variable to upload pipeline stage

ae750d5

jeancochrane temporarily deployed to deploy December 1, 2023 15:58 — with GitHub Actions Inactive

Partition Quarto performance report S3 uploads by year

3140824

jeancochrane had a problem deploying to deploy December 1, 2023 17:39 — with GitHub Actions Failure

jeancochrane commented Dec 1, 2023

View reviewed changes

jeancochrane requested a review from dfsnow December 1, 2023 17:53

dfsnow reviewed Dec 1, 2023

View reviewed changes

jeancochrane added 3 commits December 1, 2023 13:28

Strip everything after the first period in README feature table notes

25c8d91

Clean up some typos in README

7458bab

Generalize Updating R dependencies section of the README

5da06da

jeancochrane had a problem deploying to deploy December 1, 2023 19:33 — with GitHub Actions Failure

jeancochrane and others added 2 commits December 1, 2023 19:43

Generate tictoc timings for finalize pipeline stage

31dc99d

Rerender README.md

18cfbce

jeancochrane force-pushed the 24-infra-updates-generate-and-publish-a-quarto-doc-with-performance-results-on-each-model-run branch from 8ae6375 to 18cfbce Compare December 1, 2023 19:43

jeancochrane had a problem deploying to deploy December 1, 2023 19:45 — with GitHub Actions Failure

jeancochrane requested a review from dfsnow December 1, 2023 19:50

dfsnow approved these changes Dec 1, 2023

View reviewed changes

Merge branch 'master' into 24-infra-updates-generate-and-publish-a-qu…

049a642

…arto-doc-with-performance-results-on-each-model-run

jeancochrane had a problem deploying to deploy December 1, 2023 19:57 — with GitHub Actions Failure

jeancochrane merged commit 8bed163 into master Dec 1, 2023
3 of 4 checks passed

jeancochrane deleted the 24-infra-updates-generate-and-publish-a-quarto-doc-with-performance-results-on-each-model-run branch December 1, 2023 19:57

This was referenced Dec 4, 2023

Update packages, drop unused code, ingest stage running w/o error #61

Merged

Add support for Quarto reporting ccao-data/model-condo-avm#15

Closed

Add support for Quarto reporting ccao-data/model-condo-avm#16

Merged


		The process for updating model report dependencies is more complex, since it requires the use of a separate `reporting` profile:

		1. Run `Sys.setenv(RENV_PROFILE = "reporting")` to set the renv profile to `reporting`

	was moved to [pipeline/06-export.R](pipeline/07-export.R).
	was moved to [pipeline/07-export.R](pipeline/07-export.R).

	operations out into \[pipeline/06-upload.R\].
	operations out into [pipeline/06-upload.R](pipeline/06-upload.R).

		# defined separately, so this script can't be sure that it is error-free, and
		#

Generate and publish a quarto doc with performance results on each model run #62

Generate and publish a quarto doc with performance results on each model run #62

Conversation

jeancochrane commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented Dec 1, 2023

dfsnow left a comment

Choose a reason for hiding this comment

jeancochrane commented Nov 24, 2023 •

edited

Loading