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 renv lockfile for development dependencies #65

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

dfsnow
Copy link
Member

@dfsnow dfsnow commented Dec 4, 2023

This PR cleans up some of the renv dependencies used in the pipeline, adds some further copy re: dependency management to the README, and snapshots the dependencies used in previously ignored stages.

Comment on lines +1 to +8
Config/renv/profiles/dev/dependencies:
DBI,
igraph,
lubridate,
openxlsx,
readr,
rmarkdown,
RJDBC
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the additional dependencies needed for 00-ingest.R, 07-export.R, 08-api.R, and building README.Rmd.

Comment on lines +774 to +780
### Using Lockfiles for Local Development

When working on the model locally, you'll typically want to install non-core dependencies _on top of_ the core dependencies. To do this, simply run `renv::restore("<path_to_lockfile")` to install all dependencies from the lockfile.

For example, if you're working on the `ingest` stage and want to install all its dependencies, start with the main profile (run `renv::activate()`), then install the `dev` profile dependencies on top of it (run `renv::restore("renv/profiles/dev/renv.lock")`).

> :warning: WARNING: Installing dependencies from a dev lockfile will **overwrite** any existing version installed by the core one. For example, if `ggplot2@3.3.0` is installed by the core lockfile, and `ggplot2@3.2.1` is installed by the dev lockfile, renv will **overwrite** `ggplot2@3.3.0` with `ggplot2@3.2.1`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically exactly what is happening inside the Docker container. It's a little non-standard but works fine since this is only for our use.

@@ -1,6 +1,6 @@
{
"R": {
"Version": "4.2.2",
"Version": "4.3.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

Version bump on R to match the version in the Docker container.

"ppm.enabled": null,
"ppm.ignored.urls": [],
"r.version": [],
"snapshot.type": "explicit",
Copy link
Member Author

Choose a reason for hiding this comment

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

You can set the snapshot type for the entire profile once it's loaded, such that renv::snapshot() for the dev profile will only ever capture the dependencies in the DESCRIPTION file, even without type = "explicit".

Copy link
Contributor

Choose a reason for hiding this comment

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

[Suggestion, non-blocking] This is great! Perhaps we could update the README steps for updating lockfiles to remove the suggestion to pass the type flag to snapshot calls?

4. Run `renv::snapshot(type = "explicit")` to update the reporting lockfile with the dependencies defined in the `DESCRIPTION` file

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehhh, even though it does it automatically now I feel like it's a good idea to signal that it differs from what is normally happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was deprecated (replaced by settings.json).

@@ -9,7 +9,7 @@
"Depends",
"LinkingTo"
],
"ppm.enabled": null,
"ppm.enabled": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

This just gives preference to using binary packages from PPM instead of source tarballs.

@dfsnow dfsnow marked this pull request as ready for review December 4, 2023 21:26
@dfsnow dfsnow marked this pull request as draft December 4, 2023 21:53
@dfsnow dfsnow marked this pull request as ready for review December 4, 2023 21:56
Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

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

Very nice!

"ppm.enabled": null,
"ppm.ignored.urls": [],
"r.version": [],
"snapshot.type": "explicit",
Copy link
Contributor

Choose a reason for hiding this comment

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

[Suggestion, non-blocking] This is great! Perhaps we could update the README steps for updating lockfiles to remove the suggestion to pass the type flag to snapshot calls?

4. Run `renv::snapshot(type = "explicit")` to update the reporting lockfile with the dependencies defined in the `DESCRIPTION` file

@dfsnow dfsnow removed the request for review from wrridgeway December 4, 2023 23:14
@dfsnow dfsnow merged commit c5b7d6a into master Dec 4, 2023
4 checks passed
@dfsnow dfsnow deleted the dfsnow/update-dev-deps branch December 4, 2023 23:14
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.

2 participants