Skip to content

ensure that pineappl and lhapdf exist before installing vrap#152

Merged
scarlehoff merged 2 commits into
masterfrom
install_vrap_after_pineappl
Aug 27, 2022
Merged

ensure that pineappl and lhapdf exist before installing vrap#152
scarlehoff merged 2 commits into
masterfrom
install_vrap_after_pineappl

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

Closes #147

@scarlehoff
Copy link
Copy Markdown
Member Author

vrap might need an actual installation of lhapdf instead of the python package which might not work well together, how should we deal with this @alecandido ?

If the user wants vrap and no lhapdf is found in the system, uninstall the python package and then install from source? Or rather have a msg saying "please, install lhapdf by yourself, we don't want to maintain that"?

@alecandido
Copy link
Copy Markdown
Contributor

The function to install LHAPDF I believe is still there:
https://github.com/NNPDF/runcards/blob/91614524f1efdf171a01243749bd23de1daf9638/pinefarm/install.py#L281-L327

So scratch the package dependency and let's go for the usual one.

Unfortunately, the package was already falling short, because it doesn't set up the data location (part of install procedure, done with make install), and was not even exposing the CLI (thus I was using lhapdf_management, that is temporarily dead).

I'd like to have a new package to do all these stuffs, but I do not decide on my own (nor at the moment is a priority/I have time to invest on it). Let's scratch packages for LHAPDF (both, actually) and let's go for the officially supported way through autotools (a bit more painful and less integrated, but it was already working before...).

@scarlehoff
Copy link
Copy Markdown
Member Author

ok, i'll remove the lhapdf python packages then

@scarlehoff
Copy link
Copy Markdown
Member Author

Mm, not that easy. lhapdf is a top level import for various things so solving that require redesigning a bit many pieces.

@scarlehoff
Copy link
Copy Markdown
Member Author

I'm happy like this. I guess the user might get a second installation of lhapdf if they want to use vrap but that's just a fact of life.

@scarlehoff scarlehoff requested a review from alecandido August 27, 2022 18:01
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #152 (a562b51) into master (54d4a55) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
- Coverage   29.53%   29.50%   -0.03%     
==========================================
  Files          23       23              
  Lines        1141     1142       +1     
==========================================
  Hits          337      337              
- Misses        804      805       +1     
Flag Coverage Δ
unittests 29.50% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pinefarm/install.py 20.39% <0.00%> (-0.14%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment thread pinefarm/install.py
@alecandido
Copy link
Copy Markdown
Contributor

Ok, the PR is good enough for me, if it solves the problem we can also merge it.

The two things that remain:

  1. LHAPDF packages never fully worked, we should go back to a regular installation (but yes, possibly we need to scope some imports); this might still be breaking stuffs, since LHAPDF might be detected but "the wrong one"
  2. I'm not fully happy about the "dependencies": they are a bit hidden inside the functions; nothing we need to deal now, but maybe I'd implement a very basic "dependency manager": a dictionary (containing mutual dependencies), and when start installing things we populate a list, and iterate over it to install stuffs sorted (it should be simple enough, but more explicit)

@scarlehoff
Copy link
Copy Markdown
Member Author

a dictionary (containing mutual dependencies), and when start installing things we populate a list, and iterate over it to install stuffs sorted (it should be simple enough, but more explicit)

We can use some catchy name for it. Mmm.
Something like... ValidPine? 😇

@scarlehoff scarlehoff merged commit 7d3747b into master Aug 27, 2022
@scarlehoff scarlehoff deleted the install_vrap_after_pineappl branch August 27, 2022 18:33
@alecandido
Copy link
Copy Markdown
Contributor

We can use some catchy name for it.
Mmm. Something like... ValidPine? 😇

I said "simple" and "explicit" :)

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.

Check for pineappl before installing vrap

3 participants