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

Check Dependencies #1028

Closed
wants to merge 4 commits into from
Closed

Check Dependencies #1028

wants to merge 4 commits into from

Conversation

pfuehrlich-pik
Copy link
Contributor

Purpose of this PR

With this PR, dependency version requirements from DESCRIPTION are checked at the beginning of each R session via lucode2::checkDeps() in .Rprofile. If an installed package version is older than the required version there is a warning and instructions to update.

Sidenote: I re-added the Package: remindmodel line to DESCRIPTION, because some tools (e.g. pak) require this line to actually use the DESCRIPTION.

Type of change

(Make sure to delete from the Type-of-change list the items not relevant to your PR)

  • Bug fix
  • Refactoring
  • New feature
  • Minor change (default scenarios show only small differences)
  • Fundamental change
  • This change requires a documentation update

Checklist:

  • My code follows the coding etiquette
  • I have performed a self-review of my own code

@mikapfl
Copy link
Contributor

mikapfl commented Oct 21, 2022

Hi,

thanks, I have a couple of questions.

  1. This doesn't actually stop people, just emit a warning. In a test run with a too-old gms, this warning can be easily missed, the slurm config question proceeds as usual and then the run breaks due to missing options in gms. Should we instead check the dependencies in the start scripts before submitting and just stop if the dependencies are not fulfilled?
  2. What would be the one-stop solution to get to a "good" state? In my repo, I always get the warning goxygen is required, but not installed - please install it., but I don't seem to find a command to just automatically install everything that is required. It is annoying to have to renv::install("goxygen") by hand.

Cheers

Mika

@pfuehrlich-pik
Copy link
Contributor Author

  1. Could you ask that question in the next remind meeting?
  2. There's probably multiple ways, each with their own pitfalls. One way would be renv::install() without arguments.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Oct 21, 2022

Should we instead check the dependencies in the start scripts before submitting

Yes. No need to ask in the REMIND meeting, that will just devolve into 15 minutes of pointless discussion.

and just stop if the dependencies are not fulfilled?

I think lucode2::checkDeps() and lucode2::checkRequirement() could be improved in some regards. I would suggest to

  • have checkRequirement() just return the state of the requirement, either TRUE if it is met, or what goes currently into the warning ("package not installed" or "requirement not met")
  • have checkDeps() take an argument on what to do:
    • "warn": basically what it does now, but without the call signature from somewhere inside Map() which is likely to confuse people
    • "stop": throw an error; what @mikapfl is getting at
    • "pass": invisibly return the state of all packages (for use in other functions)

I can implement that, but didn't want to just start fiddling with it. Is there any code dependent on checkDeps() besides this yet? @pfuehrlich-pik

@pfuehrlich-pik
Copy link
Contributor Author

Sounds good, feel free to go ahead and implement that. checkDeps() is not used anywhere yet.

@orichters
Copy link
Contributor

Sounds good. Maybe add ask as option 4, asking the user whether he wants to run the necessary update now but also allows him to just say no.

@mikapfl
Copy link
Contributor

mikapfl commented Oct 21, 2022

Sounds good, feel free to go ahead and implement that. checkDeps() is not used anywhere yet.

Cool, will do that.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

Sounds good. Maybe add ask as option 4, asking the user whether he wants to run the necessary update now but also allows him to just say no.

Can you put that into a feature request? This will require us Pascal to figure out how to install specific package versions automatically.

@mikapfl mikapfl mentioned this pull request Oct 21, 2022
7 tasks
@mikapfl
Copy link
Contributor

mikapfl commented Oct 21, 2022

We'll do this via start scripts instead, see #1030

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.

None yet

4 participants