Skip to content

Repair renv.lock #709

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

Merged
merged 6 commits into from
Dec 13, 2023
Merged

Repair renv.lock #709

merged 6 commits into from
Dec 13, 2023

Conversation

Jeff-Thompson12
Copy link
Collaborator

Addresses #690

@mayank-procogia
Copy link

mayank-procogia commented Nov 29, 2023

Hi @Jeff-Thompson12, I tried with the updated branch jt-690-repair_renvlock and failed to successfully install all the packages.

Please see the error below -

renv::restore() Error - Installation of Matrix failed

------------------------------------------------------------------------------ 
R was unable to find one or more FORTRAN libraries during compilation.
This often implies that the FORTRAN compiler has not been properly configured.
Please see https://stackoverflow.com/q/35999874 for more information.

Reason(s):
- 'ld: library not found for -lgfortran'
install of package 'Matrix' failed [error code 1]

My current system (Mac) specifications -

Sys.info()

                                                                                                  sysname 
                                                                                                "Darwin" 
                                                                                                 release 
                                                                                                "21.6.0" 
                                                                                                 version 
"Darwin Kernel Version 21.6.0: Sun Nov  6 23:29:57 PST 2022; root:xnu-8020.240.14~1/RELEASE_ARM64_T8101" 
                                                                                                nodename 
                                                                             "Mayanks-MacBook-Pro.local" 
                                                                                                 machine 
                                                                                                "x86_64" 
                                                                                                   login 
                                                                                                  "root" 
                                                                                                    user 
                                                                                         "mayankagrawal" 
                                                                                          effective_user 
                                                                                         "mayankagrawal" 

sessionInfo()

R version 4.2.0 (2022-04-22)
Platform: x86_64-apple-darwin17.0 (64-bit)
Running under: macOS Monterey 12.6.2

Matrix products: default
LAPACK: /Library/Frameworks/R.framework/Versions/4.2/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods   base     

loaded via a namespace (and not attached):
[1] BiocManager_1.30.22 compiler_4.2.0      tools_4.2.0         renv_1.0.0      

@mayank-procogia
Copy link

Hi @Jeff-Thompson12,

I hypothesized that updating my renv.lock exclusively through PPM might resolve this issue, taking inspiration from your recent commits with this issue.

After observing changes to your renv.lock file in jt-690-repair_renvlock, I adapted my renv.lock to utilize PPM as the sole repository for R package downloads and installations.

I conducted three tests, involving cloning, executing renv::restore(), and launching the application, and all tests were successful.

Please review the comparing changes between pharmaR/riskassessment:master and mayank-procogia/riskassessment:ppm_renv_fix.

Branch: mayank-procogia/riskassessment:ppm_renv_fix

@mayank-procogia
Copy link

As a part of above hypotheses, I went ahead and forked jt-690-repair_renvlock to mayank-procogia/riskassessment:jt_690_ppm_renv_fix to have all the latest changes from this working branch.

I then updated renv.lock from commit a4d9570 of mayank-procogia/riskassessment/dev_renv_fix. This fix uses PPM as single repo only, and uses the updated pkgs of pkgload(1.3.3), rlang(1.1.1) and svglite(1.2.3) which in turn auto updates other packages as renv fix. Note that this is a downgrade of other updated packages from your recent commit.

In essence, this is similar to the previous step but with the other additional commits in this branch for merging, if accepted.

I have tested it out twice in my local involving cloning, executing renv::restore(), and launching the application, and all tests were successful.

Please review comparing changes between pharmaR/riskassessment:jt-690-repair_renvlock and mayank-procogia/riskassessment:jt_690_ppm_renv_fix

@mayank-procogia
Copy link

mayank-procogia commented Nov 29, 2023

Second fix: Downgrading required packages in renv.lock with respect to your latest commits in jt-690-repair_renvlock branch.

Downgraded Packages Matrix(1.4-1), nlme(3.1-157), and svglite(1.2.3)

As a result, the dependency tree was repaired during package installation:

- crul              [1.4.0]
- fontBitstreamVera [0.1.1]
- fontLiberation    [0.1.0]
- fontquiver        [0.2.1]
- gdtools           [0.3.3]
- gfonts            [0.2.0]
- httpcode          [0.3.0]

Please review comparing changes between pharmaR/riskassessment:jt-690-repair_renvlock and mayank-procogia/riskassessment:/jt_690_renv_Matrix

Branch: mayank-procogia/riskassessment:jt_690_renv_Matrix forked from pharmaR/riskassessment:jt-690-repair_renvlock

Deployed Content on ProCogia RS Connect for testing:

Testing creds:

  • username: ADMIN
  • pwd: Admin@1

Note:

  • When deploying to ProCogia RS Connect with the latest changes in pharmaR/riskassessment:jt-690-repair_renvlock it deploys successfully, however it fails on my local (Mac) system. Link
  • Using mayank-procogia:riskassessment:jt_690_renv_Matrix for deployment is successful on both local and RSC. Link

@Jeff-Thompson12
Copy link
Collaborator Author

@mayank-procogia it might be helpful to explain a little background into why the renv.lock file has been changed. In the project we always had a mix of packages and download dates (i.e. even though we originally had a snapshot set, not all packages were from that time point). When MRAN got decommissioned, we had to update renv. This caused issues with downloading packages that required versions post the snapshot date (e.g. you had to run `utils::install.packages("shinyTree") to install the package). We then chose to remove the snapshot date. The deployment sans snapshot date was working on our windows systems and the Linux systems on both GHAs and our deployments.

I say all this to express that in my opinion while aligning all the package versions to a snapshot date in PPM is desirable, I'm not really of the opinion that we should be hunting for package versions outside of the snapshot for system specific installations. Our design philosophy has been to develop with server deployment in mind.

Did you try to resolve Fortran issue you ran into on your Macbook? I'm not sure how feasible that would be.

@AARON-CLARK do you have any comment/opinion you would like to share?

@mayank-procogia
Copy link

Hi @Jeff-Thompson12,

I attempted to resolve the Fortran issue on my MacBook but hit a dead end.

I've encountered similar problems with the latest versions of svglite in other Shiny projects and found that downgrading to a stable version (1.2.3) often resolved the issue. Similarly, downgrading Matrix(1.4-1) and nlme(3.1-157) addressed C++ and Fortran issues on Mac, avoiding the need for hot fixes. I have experienced similar fixes for other applications pacakges snapshot while working on Windows too. My goal was to streamline the process with renv ensuring seamless handling during the initial clone and renv restore on all of the systems.

The initiative to identify and modify packages in the renv.lock file was undertaken to ensure compatibility across Windows, Linux, and Mac systems.

For now, we can overlook this additional fixes, as the deployment on Linux, Windows, GHAs and RS Connect works as expected with your fix and PPM date snapshot. This suffices for us to deploy it on a Linux based infrastructure with persistent storage, in line with riskassessment future plans.

I have a meeting with Aaron tomorrow and will keep you updated after our discussion.

Thanks

@mayank-procogia
Copy link

mayank-procogia commented Dec 4, 2023

Hi @Jeff-Thompson12,

We're sticking with your renv.lock fix as the entire dev team syncs with it on Windows. Mac support may come later based on user requests. Downgraded versions are anyways documented in this issue.

Also, FYI, CC'd you in an email to Aaron today with the updates.
Please check your gmail inbox associated with your personal GitHub account.

Thank you

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c6a9021) 77.17% compared to head (f2782c4) 77.17%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #709   +/-   ##
=======================================
  Coverage   77.17%   77.17%           
=======================================
  Files          33       33           
  Lines        4872     4872           
=======================================
  Hits         3760     3760           
  Misses       1112     1112           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jeff-Thompson12 Jeff-Thompson12 marked this pull request as ready for review December 6, 2023 21:19
@Jeff-Thompson12
Copy link
Collaborator Author

One thing I do not like about this renv.lock file is that it downgrades {shinytest2} and {chromote} which does not affect the GHAs but does affect test development since app$view() does not work. Developers can of course update those packages locally when needed. I still think the benefits outweigh the cons.

Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

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

Per discussion, this PR looks good. Also, I pulled down and tested with no noticeable issues. Thanks @Jeff-Thompson12.

@AARON-CLARK AARON-CLARK merged commit 93f80f1 into dev Dec 13, 2023
@AARON-CLARK AARON-CLARK deleted the jt-690-repair_renvlock branch March 12, 2024 12:33
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.

3 participants