-
-
Notifications
You must be signed in to change notification settings - Fork 33
Bump to R 4.5 & Bioconductor 3.22 #884
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
Conversation
|
Ah, well this Docker build fail checks out. Going to be hard to build for 4.5/3.22 when the Dockerfile isn't getting that version to begin with! |
known bug: https://github.com/rstudio/renv/blob/main/NEWS.md |
thanks! i had only looked at their issues, not their release notes. so that's good. Noting also I tried to build locally before pushing the last 2 commits, but kept hitting a python error. I sadly see that the GHA hits the same error: https://github.com/AlexsLemonade/training-modules/actions/runs/21218044606/job/61044746608?pr=884#step:8:11468 edit - seems to be a bioc 3.22 base image situation, e.g. found this https://pythonspeed.com/articles/externally-managed-environment-pep-668/ |
jashapiro
left a comment
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 python error has an easy and a hard solution. For now I think we can try the easy but dangerous one and then we can consider the best way to implement the hard but safe one.
Basically, we can force install the Python packages at the system level, which is what we are doing, but has always been a not-great idea.
A longer term solution will be to install the packages in a separate environment and then update the skel files to direct all users to that environment. Let's file a separate issue to come back to that.
I also suggested here updating the base image to match the bioconductor version
Thanks for weighing in! I uncovered this right before hopping over to a meeting, so just now getting back to it. Your comment is happily along the lines of what I was thinking - we want a more robust solution here, but that's a whole thing™️ that seems beyond the scope of bumping R versions. We'll live on the edge for now, and come back from the edge later. (is this a pun re- |
Co-authored-by: Joshua Shapiro <josh.shapiro@ccdatalab.org>
|
no replies to this, but possibly relevant COMBINE-lab/salmon#984 |
jashapiro
left a comment
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.
no replies to this, but possibly relevant COMBINE-lab/salmon#984
Good find. It actually looks like this was updated in later point releases (that are tagged but not "releases"), so I suggested the version bump here. It also seems to require libzstd, so I added that as well.
Good find to everyone. |
Co-authored-by: Joshua Shapiro <josh.shapiro@ccdatalab.org>
jashapiro
left a comment
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.
LGTM. I admit I did not go through all of the renv changes, but I am trusting that there is nothing too crazy in there.
| # Temporary fix for broken(?) RSamtools package | ||
| RUN Rscript -e "install.packages('BiocManager'); BiocManager::install('Rsamtools')" |
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 that this works again
Closes #869
This PR updates the renv environment to 4.5 with associated bioc 3.22. For this, I removed the last renv environment and snapshotted in a new one for the versions of interest. Matching our current renv.lock file, I ensured that we're pointing to the ppm link for the CRAN repo.
After snapshotting, I compared with
setdiffpackages that were in 4.4 vs now in 4.5 lock files. I made a little spreadsheet here to confirm that it all makes sense, and it does! https://docs.google.com/spreadsheets/d/1JAKuYGS6_mdzga06urdE9RsrOSsjO-2zMxdoAWHQtOk/edit?usp=sharingThe first sheet contains packages in 4.4 but not the 4.5 lockfile. It might be that we want some of these in the
dependencies.Rfile, but we can't say for sure at this time. We'll probably want to revisit when we journey into how the version bump affected module notebooks. Note that otherwise,dependencies.Rlooks fine to stay as is for now.That said, note this new aspect of renv's settings: https://rstudio.github.io/renv/reference/settings.html?q=snapshot.dev#snapshot-dev.
Finally, this is kind of annoying. Following the code in renv/activate.R that is throwing this, I don't immediately see why this would happen because versions, indeed, match. Do you see the "culprit"?