search

pharmaR / riskassessment

Risk Assessment Demo App: https://rinpharma.shinyapps.io/riskassessment
https://pharmar.github.io/riskassessment/
Other
98 stars 25 forks source link

Repair renv.lock #709

Closed Jeff-Thompson12 closed 6 months ago

Jeff-Thompson12 commented 7 months ago

Addresses #690

mayank-procogia commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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:

Note:

Jeff-Thompson12 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Jeff-Thompson12 commented 7 months ago

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.