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

Use eko.runner.managed.solve #151

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Use eko.runner.managed.solve #151

merged 1 commit into from
Feb 7, 2024

Conversation

felixhekhorn
Copy link
Contributor

Use the new "jets runner"

Remember that there are still upcoming changes in eko NNPDF/eko#296 which might affect pineko ...

cc @alecandido @scarlehoff

@felixhekhorn felixhekhorn added the enhancement New feature or request label Jan 30, 2024
@scarlehoff
Copy link
Member

But should this be default if it is slower? Maybe it should be a cmdlije option.

@felixhekhorn
Copy link
Contributor Author

But should this be default if it is slower? Maybe it should be a cmdlije option.

I would argue yes, because (as you experienced yourself) the current default may not converge always and it shouldn't be much slower: yes, there are more write-to-disk operations, but compared to the numerical integration (~mins to hours) this should be negligible (to be proven, but I'm quite sure)

@niclaurenti
Copy link

But should this be default if it is slower? Maybe it should be a cmdlije option.

With the same number of cores it maybe be slightly slower, but the fact that it uses less memory allows us to increase the number of cores to make faster. (This is what I did to compute the evolution eko for QED)

@alecandido
Copy link
Member

I would argue yes, because (as you experienced yourself) the current default may not converge always and it shouldn't be much slower: yes, there are more write-to-disk operations, but compared to the numerical integration (~mins to hours) this should be negligible (to be proven, but I'm quite sure)

Yes, and also it was just a transition. We don't want to maintain both, so at some point the legacy one will be fully dropped.

@alecandido
Copy link
Member

alecandido commented Jan 30, 2024

With the same number of cores it maybe be slightly slower, but the fact that it uses less memory allows us to increase the number of cores to make faster. (This is what I did to compute the evolution eko for QED)

In principle, it could even be distributed on multiple nodes. The computation is happening in two steps:

  1. (recipes to detail what to compute are generated)
  2. many independent parts are integrated (the heavy lift)
  3. the parts are merged in full-blown operators

We never finished implementing the infrastructure for splitting recipes and merging parts, but it just boils down to file system operations (moving files around).
Then you could split your point 1. computation in as many batches as you wish, and distribute wherever you want.

@scarlehoff
Copy link
Member

Please wait to merge because I'm still not sure this solves the problem.

@andreab1997
Copy link
Contributor

Please wait to merge because I'm still not sure this solves the problem.

I guess this worked at the end right? So we can probably merge.

@scarlehoff
Copy link
Member

Well, I was not able to create the fktables so not sure if the eko is correct or anything but I guess other people has already tested this a lot?

@andreab1997
Copy link
Contributor

Well, I was not able to create the fktables so not sure if the eko is correct or anything but I guess other people has already tested this a lot?

Yes of course, we know that it is possible, you just need more memory. Of course it is not ideal but it is not related to this PR.

@felixhekhorn
Copy link
Contributor Author

Can this be merged now?

@alecandido
Copy link
Member

@felixhekhorn since Pineko is the main EKO user, if this works for Pineko, shouldn't you make the managed.solve just the default in EKO itself, and throw away the legacy one? (together with eko.evolution_operator.grid)

@felixhekhorn
Copy link
Contributor Author

@felixhekhorn since Pineko is the main EKO user, if this works for Pineko, shouldn't you make the managed.solve just the default in EKO itself, and throw away the legacy one? (together with eko.evolution_operator.grid)

We may want to do this at some point, but since I consider this is a major change (although the user should(tm) never notice), I would delay that

@alecandido
Copy link
Member

We may want to do this at some point, but since I consider this is a major change (although the user should(tm) never notice), I would delay that

I agree it's a major change, but eventually it's the same if you do it in Pineko or EKO :P

I know that EKO has also other users beyond Pineko, and that you'd like to group this major change with other ones. But if the alternative is to release the major in a couple of months, you could well release a major now with just that, and another one in a couple of months.

@felixhekhorn felixhekhorn merged commit 210da28 into main Feb 7, 2024
5 checks passed
@felixhekhorn felixhekhorn deleted the use-eko-managed-solve branch February 7, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants