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.

Sign up for GitHub

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

Jump to bottom

clean up renv usage to fix incompatibility between stored CRAN repositories and renv version. update README.md files. regenerate renv.lock files #29

Merged
merged 2 commits into from
Feb 6, 2025
Merged

clean up renv usage to fix incompatibility between stored CRAN repositories and renv version. update README.md files. regenerate renv.lock files #29

merged 2 commits into from
Feb 6, 2025

Conversation

matthewcornell
Copy link
Member

@matthewcornell matthewcornell commented Feb 6, 2025 •
edited
Loading

Fixes issue #28 with big help from @zkamvar . Once this is merged then we should:

  • continue cleaning up by incorporating @zkamvar suggestions re: using install2.r and installGithub.r instead of renv::install to install packages

Then:

@matthewcornell
issue #28: clean up renv usage to fix incompatibility between stored …
6220464 
…CRAN repositories and renv version. update README.md files. regenerate renv.lock files
@matthewcornell matthewcornell changed the title issue #28: clean up renv usage to fix incompatibility between stored CRAN repositories and renv version. update README.md files. regenerate renv.lock files clean up renv usage to fix incompatibility between stored CRAN repositories and renv version. update README.md files. regenerate renv.lock files Feb 6, 2025
zkamvar
zkamvar approved these changes Feb 6, 2025
View reviewed changes
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I've confirmed that this works by building the flu_ar2 image. Once caveat I found: it is very possible to build the container without using --platform=linux/amd64, but you will find that it builds MUCH slower because the p3m binary packages are not available for arm architecture. I discovered this because I used the docker build command found at the beginning of the README.

I've added suggestions for documentation to include the sources we used to get to these conclusions, but these are optional.

There are a couple of issues that still need to be addressed after this PR:

  1. The installations of the GitHub packages from reichlab negates the effort to install released versions of hubverse packages because they always update to the latest version (as the Remotes: field always points to the main branch). This cannot be solved by using renv::install(). The code in the development are considered stable, but it does not match what the models will be validated against.
  2. The workflow for building and deploying the docker containers is a manual one and can be automated, but this would require tests to confirm that it works.

Both 1 and 2 are important in the event that the containers need to be rebuilt or updated (e.g. a severe bug in any of the software used or a change to the hub requires newer versions of hubverse packages).

README.md
- install required R libraries. NB: these will vary depending on the model (see each model's `README.md` for the actual list). For example:
1. start a fresh temporary [rocker/r-ver:4.4.1](https://hub.docker.com/layers/rocker/r-ver/4.4.1/images/sha256-f3ef082e63ca36547fcf0c05a0d74255ddda6ca7bd88f1dae5a44ce117fc3804) container via:
```bash
docker run --rm -it --name temp_container rocker/r-ver:4.4.1 /bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

The rocker containers are built on amd64 and if you don't specify a platform, you get this lovely warning on arm Macs:

WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
Suggested change
docker run --rm -it --name temp_container rocker/r-ver:4.4.1 /bin/bash
docker run --platform linux/amd64 --rm -it --name temp_container rocker/r-ver:4.4.1 /bin/bash

Copy link
Member Author

Choose a reason for hiding this comment

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

The rocker containers are built on amd64 and if you don't specify a platform, you get this lovely warning on arm Macs: ...

Thanks for the heads up. Interestingly, I don't see that warning on my Mac. Generally I've been omitting the --platform flag and assuming the reader knows which to use, with the exception of the "To publish the image" section, where I specify amd64.

README.md Outdated
docker run --rm -it --name temp_container rocker/r-ver:4.4.1 /bin/bash
```
2. install the required OS libraries and applications (see "install general OS utilities" and "install OS binaries required by R packages" in the [Dockerfile](Dockerfile))
3. specify repo via:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
3. specify repo via:
3. specify the [p3m repository snapshot to a particular date](https://p3m.dev/client/#/repos/cran/setup?distribution=ubuntu-22.04&r_environment=other&snapshot=2025-02-05) (this allows binary packages to be installed for faster builds) (see the [rocker-project guidance for switching the default CRAN mirror](https://rocker-project.org/images/versioned/r-ver.html#switch-the-default-cran-mirror)

Dockerfile
@@ -76,7 +76,8 @@ RUN pip3 install -r requirements.txt
# install required R packages using renv
COPY "${MODEL_DIR}/renv.lock" ./
ENV RENV_PATHS_LIBRARY="renv/library"
RUN Rscript -e "install.packages('renv', repos = c(CRAN = 'https://cloud.r-project.org'))"
RUN /rocker_scripts/setup_R.sh https://p3m.dev/cran/__linux__/jammy/2025-02-05
Copy link
Member

Choose a reason for hiding this comment

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

Adding comments for reference

Suggested change
RUN /rocker_scripts/setup_R.sh https://p3m.dev/cran/__linux__/jammy/2025-02-05
# Guidance for setting default CRAN mirror: https://rocker-project.org/images/versioned/r-ver.html#switch-the-default-cran-mirror
# Posit Public Package Manager setup: https://p3m.dev/client/#/repos/cran/setup
RUN /rocker_scripts/setup_R.sh https://p3m.dev/cran/__linux__/jammy/2025-02-05

Dockerfile Outdated
@@ -76,7 +76,8 @@ RUN pip3 install -r requirements.txt
# install required R packages using renv
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# install required R packages using renv
# install required R packages using renv
# see https://rstudio.github.io/renv/articles/docker.html

@matthewcornell matthewcornell mentioned this pull request Feb 6, 2025
Open
@matthewcornell
Copy link
Member Author

There are a couple of issues that still need to be addressed after this PR: ...

Good points. I've created this issue to address them: [change docker setup to install released versions of hubverse packages #30]

@matthewcornell matthewcornell merged commit 4f90ae1 into main Feb 6, 2025
@matthewcornell matthewcornell deleted the mc/fix-build-errors/28 branch February 6, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkamvar zkamvar zkamvar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewcornell @zkamvar