search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Calling CLI convolute takes 8 minutes #134

Closed felixhekhorn closed 11 months ago

felixhekhorn commented 2 years ago

On dom I did

  1. git pull

  2. $ cargo install --features=fastnlo --path pineappl_cli --root /media/FK/env as written in the README. This generates a bunch of warnings (-Wunused-parameter, -Wmaybe-uninitialized) which maybe you want to avoid and eventually

    Finished release [optimized] target(s) in 55.74s                                                                                                                                                               
Replacing /media/FK/env/bin/pineappl                                                                  
Replaced package `pineappl_cli v0.5.0 (/media/FK/pineappl/pineappl_cli)` with `pineappl_cli v0.5.2 (/media/FK/pineappl/pineappl_cli)` (executable `pineappl`)

  3. check the size as suggested: 

    $ du -h $(which pineappl)                                                                                   
5.7M    /media/FK/env/bin/pineappl

    so no, it is bigger then 2M

  4. double check the things is actually replaced:

    $ ls /media/FK/env/bin/ -l | grep pineappl
-rwxr-xr-x 1 felix      nnpdf  5898240 Mar 30 17:18 pineappl

  5. check the grid size:

    $ du -h ../fktables/data/appl_subfktables/200-CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 
328M    ../fktables/data/appl_subfktables/200-CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4

now measuring convolute I get:

$ (timedatectl | head -n 1) && (pineappl convolute data/appl_subfktables/200-CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 NNPDF40_nnlo_as_01180 > /dev/null ) && (timedatectl | head -n 1)
               Local time: Wed 2022-03-30 17:25:22 CEST
               Local time: Wed 2022-03-30 17:33:22 CEST
felixhekhorn commented 2 years ago

Interestingly enough python does not to seem affected:

$ (timedatectl | head -n 1) && (pineko compare data/appl_subgrids/CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 data/appl_subfktables/200-CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 2 0 NNPDF40_nnlo_as_01180 > /dev/null) && (timedatectl | head -n 1)
               Local time: Wed 2022-03-30 17:52:28 CEST
               Local time: Wed 2022-03-30 17:53:43 CEST

this sounds like truly the CLI (and not the PineAPPL library behind)

alecandido commented 2 years ago

Remember that Rust binaries are mostly statically compiled, so the version of the library might differ between Python interface and the CLI. If you're sure it's the same (because you recompiled both), than of course this does not apply.

Compilation warnings are all related to --features=fastnlo, since the fastNLO library bindings are wider than what it's strictly needed (that's not bad, since it's a first iteration and we might need it in the future). If you don't need to convert fastNLO grids, you can avoid that feature, and you shouldn't get as many warnings.

cschwan commented 2 years ago

I agree that truly seems like a performance bug, could you check that you have the same problem also for the grid at process scale?

The fastNLO warnings I can't get rid off, they originate from the fastNLO toolkit headers, but it's annoying I agree.

Remember that Rust binaries are mostly statically compiled, so the version of the library might differ between Python interface and the CLI. If you're sure it's the same (because you recompiled both), than of course this does not apply.

This is a good point, please check that. I might've introduced a performance regression at some point.

cschwan commented 2 years ago

Quick fact list: after the evolution

The last two factors are roughly consistent, so in other words the size of the grid is driven by the change of the flavour basis into the evolution basis. The difference in time is much more, so there could be something wrong with the PDF cache.

alecandido commented 2 years ago

Is there any trivial optimization that is missing for those grids? Or is there anything we can do to optimize them?

cschwan commented 2 years ago

The FK table optimization is missing, I believe (Nf4Sym).

cschwan commented 2 years ago

After this optimization step the file size drops from 328M to 150M (factor 2.2), and the execution time from 6m17.859s to 2m19.145s (factor 2.7) on my local machine.

alecandido commented 2 years ago

Much better, but still much slower than the original one. It's true we're dealing with an intrinsic property of the object itself, since evolution is entangling everything, by an unpredictable amount.

I wonder if there is anything more we can do for further optimizing it... (I don't see it manifest)

felixhekhorn commented 2 years ago

The FK table optimization is missing, I believe (Nf4Sym).

just to confirm this is true

After this optimization step the file size drops from 328M to 150M (factor 2.2), and the execution time from 6m17.859s to 2m19.145s (factor 2.7) on my local machine.

okay, I take it you can reproduce to some extend the problem locally

still, I don't understand why python does not seem to be affected - is there some easy debugging I can do?

cschwan commented 2 years ago

still, I don't understand why python does not seem to be affected - is there some easy debugging I can do?

What do you run exactly and which version of PineAPPL do you use?

felixhekhorn commented 2 years ago

as said, I did a pull so:

$ pineappl --version
pineappl v0.5.2-3-g3789a89
felixhekhorn commented 2 years ago

I agree that truly seems like a performance bug, could you check that you have the same problem also for the grid at process scale?

no at process level I get:

$ (timedatectl | head -n 1) && (pineappl convolute data/appl_subgrids/CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 NNPDF40_nnlo_as_01180 > /dev/null ) && (timedatectl | head -n 1)
               Local time: Mon 2022-04-04 11:35:58 CEST
               Local time: Mon 2022-04-04 11:36:05 CEST
$ du -h data/appl_subgrids/CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4
12M     data/appl_subgrids/CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4
cschwan commented 2 years ago

as said, I did a pull so:

$ pineappl --version
pineappl v0.5.2-3-g3789a89

But that's the CLI version, if you're using Python I need PineAPPL's Python interface version. Also what do you run exactly?

felixhekhorn commented 2 years ago

okay, so just did

$ maturin develop --release
[...]
   Compiling pineappl v0.5.2 (/media/FK/pineappl/pineappl)                                               
   Compiling numpy v0.14.1                          
   Compiling pineappl_py v0.5.2 (/media/FK/pineappl/pineappl_py)                                         
    Finished release [optimized] target(s) in 19.79s

and it seemed indeed to have done the job:

$ ls env/lib/python3.10/site-packages/pineappl -l
total 10008
-rw-r--r-- 1 felix nnpdf      516 Apr  4 11:40 bin.py 
-rw-r--r-- 1 felix nnpdf     1688 Apr  4 11:40 fk_table.py
-rw-r--r-- 1 felix nnpdf     9715 Apr  4 11:40 grid.py
-rw-r--r-- 1 felix nnpdf      762 Apr  4 11:40 import_only_subgrid.py
-rw-r--r-- 1 felix nnpdf       70 Apr  4 11:40 __init__.py
-rw-r--r-- 1 felix nnpdf      379 Apr  4 11:40 lumi.py
-rwxr-xr-x 1 felix nnpdf 10200296 Apr  4 11:40 pineappl.cpython-310-x86_64-linux-gnu.so
drwxr-sr-x 2 felix nnpdf     4096 Apr  4 11:40 __pycache__
-rw-r--r-- 1 felix nnpdf      254 Apr  4 11:40 subgrid.py
-rw-r--r-- 1 felix nnpdf      430 Apr  4 11:40 utils.py

further more

$ pip list | grep pineappl
pineappl                        0.5.2

and then rerunning the command from https://github.com/N3PDF/pineappl/issues/134#issuecomment-1083320997 (so two! convolutions) I still get the roughly one minute ...

cschwan commented 2 years ago

@felixhekhorn it seems that scale variations make the difference - I don't know why yet, but it's a start. Can you please try to convolute with the CLI with -s 1 to disable them?

felixhekhorn commented 2 years ago

@felixhekhorn it seems that scale variations make the difference - I don't know why yet, but it's a start. Can you please try to convolute with the CLI with -s 1 to disable them?

yes, this seems to be confirmed:

$ (timedatectl | head -n 1) && (pineappl convolute data/appl_subfktables/200-CMS_1JET_8TEV-CMS_1JET_8TEV.pineappl.lz4 NNPDF40_nnlo_as_01180 -s 1 > /dev/null ) && (timedatectl | head -n 1)
               Local time: Mon 2022-04-04 11:59:16 CEST
               Local time: Mon 2022-04-04 12:00:39 CEST

(still using the unoptimized grid ...)

felixhekhorn commented 2 years ago

@felixhekhorn it seems that scale variations make the difference - I don't know why yet, but it's a start. Can you please try to convolute with the CLI with -s 1 to disable them?

felixhekhorn commented 2 years ago
  • doing a local test on LHCB_DY_13TEV_DIELECTRON: I get 3s with no scale variation and 19s with full

to be more quantitative:

now, to be fair this is almost expected, since the number of convolutions scales linearly with the number of scales - so I wonder whether we look at a non-issue here ... and instead just a non-correct usage of the CLI, then I wonder whether we should somehow flag the FK tables to deny scale variations ...

cschwan commented 2 years ago

@felixhekhorn I'm still a bit concerned about the scaling, but I agree this isn't much of an Issue. Not printing scale variation when you can't reliably predict them is probably more important; I'd opened https://github.com/N3PDF/pineappl/issues/46 for that but I wonder if what I wrote is true. When we have FK tables we can certainly disable scale-variations.

felixhekhorn commented 2 years ago

I wonder whether we should somehow flag the FK tables to deny scale variations ...I wonder whether we should somehow flag the FK tables to deny scale variations ...

I think FK tables match exactly #46

cschwan commented 11 months ago

This is no longer a problem as pineappl convolute doesn't calculate scale uncertainties anymore.