NNPDF / pinecards

Runcards needed to generate PineAPPL grids for NNPDF processes
3 stars 1 forks source link

ensure that pineappl and lhapdf exist before installing vrap #152

Closed scarlehoff closed 2 years ago

scarlehoff commented 2 years ago

Closes #147

scarlehoff commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

ok, i'll remove the lhapdf python packages then

scarlehoff commented 2 years ago

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

scarlehoff commented 2 years ago

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.

codecov-commenter commented 2 years ago

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%) :arrow_down:

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%) :arrow_down:

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

alecandido commented 2 years ago

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 commented 2 years ago

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? :innocent:

alecandido commented 2 years ago

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

I said "simple" and "explicit" :)