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

Recover numba #108

Merged
merged 5 commits into from
Mar 29, 2022
Merged

Recover numba #108

merged 5 commits into from
Mar 29, 2022

Conversation

felixhekhorn
Copy link
Contributor

@felixhekhorn felixhekhorn commented Mar 18, 2022

Here I want to recover the "true" numba behaviour, i.e. compilation just-in-time (and not ahead-of-time)

  • I think it is just a matter of removing all signatures (as is done in this commit)
  • then import eko is immediate and instead running, e.g. the LHA benchmark, has some initialization cost
  • tests are still running* and the LHA benchmark is still working - so I think this works

Please have a look and then we can merge back to #83

*actually, they are not - because https://github.com/N3PDF/eko/blob/606138fcfc31e0364457ab49e9e9557597e49a42/src/eko/evolution_operator/__init__.py#L243 forces me to do a manual CTRL+C in the test shell - and hence I strongly recommend to revert that

PS: I think we introduced the signatures because at some point we had problems with types, i.e. we were not sufficiently casting - hopefully this is gone now ...

@giacomomagni
Copy link
Collaborator

giacomomagni commented Mar 18, 2022

Okay. It's working so fine for me.
Regarding:
https://github.com/N3PDF/eko/blob/606138fcfc31e0364457ab49e9e9557597e49a42/src/eko/evolution_operator/__init__.py#L243
I think we should just set it to one in all the instances of Operator and OperatorMatrixElement inside tests. It's useless to have more pools there.
Or maybe add an option to switch it off....

@felixhekhorn
Copy link
Contributor Author

I think we should just set it to one in all the instances of Operator and OperatorMatrixElement inside tests. It's useless to have more pools there.

I think that would work - you need to make that option accessible from the outside though

I still think this parallelization is a good a idea - since it is an immediate help for us developers as it speeds up the computation of a single eko (as we typically do), instead #105 is for users who typically request several Q2. This can addressed here or back in #83 where it was introduced

@felixhekhorn
Copy link
Contributor Author

this should correctly address #78. We can still keep #94 - or close it.

@felixhekhorn felixhekhorn added enhancement New feature or request refactor Refactor code labels Mar 18, 2022
@giacomomagni
Copy link
Collaborator

I think that would work - you need to make that option accessible from the outside though

Shall I add an option to the operator card? something like: compute_parellel: True/False

@felixhekhorn
Copy link
Contributor Author

Shall I add an option to the operator card? something like: compute_parellel: True/False

maybe yes - the name has to be more specific though I think (or maybe it's just fine, since as said #105 plays on a different level and is parallelize on the outside) ... or maybe pass directly the number of processes? with 0=all or something like this (and -1 = all-1)

@alecandido
Copy link
Member

The most important question: is the cache still working?

@alecandido
Copy link
Member

maybe yes - the name has to be more specific though I think (or maybe it's just fine, since as said #105 plays on a different level and is parallelize on the outside) ... or maybe pass directly the number of processes? with 0=all or something like this (and -1 = all-1)

Definitely, I'd put in the runcard the number of processor, this is something you always want to control.

In #105, I started splitting the operator card in several chunks, so this option can now be scoped in configs: section, that is dedicated to the internal configurations of eko.
At the moment contain part of what we believe should be in theory (ev_op... and backward_method), but eventually it should contain things like the Mellin paths (and maybe its parameters), quad precision and these kind of things.

@alecandido
Copy link
Member

Actually, there will be no clash with parallelization itself w.r.t. to #105 and related, since eko will never provide Q2 parallelization itself, but it will just split the jobs.

Jobs management should be done:

  • directly by the user, or
  • in a dedicated runner in ekobox

We have to mandatory split the jobs in eko, otherwise no one would have a handle on them, but managing the jobs is beyond eko's scope.

@felixhekhorn
Copy link
Contributor Author

The most important question: is the cache still working?

Yes, I think so - but please do a simple run yourself

(not that this has an interplay with the parallelization, since it seems you need to compile numpy in numba (numba.np) on each core)

@alecandido
Copy link
Member

this should correctly address #78. We can still keep #94 - or close it.

If there is not any longer the problem of initial slow down (i.e. slow down on import, not on run), for me, we can close even #94 (i.e. link the issue to this PR)

@felixhekhorn felixhekhorn linked an issue Mar 21, 2022 that may be closed by this pull request
3 tasks
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #108 (d5f8bba) into feature/N3LO_matching (606138f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           feature/N3LO_matching      #108   +/-   ##
=======================================================
  Coverage                 100.00%   100.00%           
=======================================================
  Files                         59        59           
  Lines                       2956      2965    +9     
=======================================================
+ Hits                        2956      2965    +9     
Flag Coverage Δ
isobench 54.87% <97.87%> (+0.13%) ⬆️
unittests 99.93% <98.93%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
src/eko/anomalous_dimensions/__init__.py 100.00% <100.00%> (ø)
src/eko/anomalous_dimensions/aem1.py 100.00% <100.00%> (ø)
src/eko/anomalous_dimensions/as1.py 100.00% <100.00%> (ø)
src/eko/anomalous_dimensions/as2.py 100.00% <100.00%> (ø)
src/eko/anomalous_dimensions/as3.py 100.00% <100.00%> (ø)
src/eko/anomalous_dimensions/harmonics.py 100.00% <100.00%> (ø)
src/eko/beta.py 100.00% <100.00%> (ø)
src/eko/evolution_operator/__init__.py 100.00% <100.00%> (ø)
src/eko/evolution_operator/grid.py 100.00% <100.00%> (ø)
src/eko/gamma.py 100.00% <100.00%> (ø)
... and 35 more

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge.
(All tests run on single core, while benchmarks are still in parallel)

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing but signatures and multiprocessing.Pool related stuffs. I'm happy to merge as well.

In case we'll want the signatures back, they are still in git anyhow, so we can recover them with reasonable to little effort in the next future (more changes more effort, of course, but even less likely we'd like to change).
Not that we should, I believe you this is working as expected, just to point that we could.

Copy link
Contributor Author

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor things - else we can merge and move on ...

src/ekomark/data/operators.py Outdated Show resolved Hide resolved
src/eko/evolution_operator/__init__.py Show resolved Hide resolved
@giacomomagni giacomomagni merged commit e6ee4bf into feature/N3LO_matching Mar 29, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/nb-njit branch March 29, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimize the amount of initial imports
3 participants