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

Improve runner API #296

Merged
merged 16 commits into from
Aug 9, 2024
Merged

Improve runner API #296

merged 16 commits into from
Aug 9, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Aug 1, 2023

The idea is to allow the definition of a suitable runner in Pineko, to make use of the Q2 granularity, and eventually drop n_integration_cores in EKO.
If we can suitably split at the level of Q2, there is no need to parallelize at the level of x, since our xgrid is not going to grow wild. Avoiding parallelization will simplify a bit the content of the eko package, and it will solve many problems encountered with Python parallelization.

To allow this, the following steps are required:

  • expose a high level API from eko.runner, such that it will be reliable outside the package itself (this will be the public runner API) -> postponed to Expose new runner API #399
  • load opened EKOs, to avoid the need of merging separately computed Q2 values, for which we only need
    • to avoid the use of the context manager (such that the folder won't be deleted)
    • to specify a given path for the folder, to be accessed even outside EKO (if None, the default, use a temporary one, as it is currently done)
    • a utility wrapper, to create the object from the folder

In a later PR, a merge utility can be added nevertheless, taking into account the scenario in which a single EKO is initialized, and copied over for parts and operators computation.
In particular, this limited merge utility will take two EKO tar files, and just copy the content of parts/ and operators/ subfolders of the second into the first one (in-place operation, to save disk - to implement the immutable operation just copy the first EKO tar beforehand).

@felixhekhorn felixhekhorn added enhancement New feature or request output Output format and management labels Aug 1, 2023
@felixhekhorn felixhekhorn mentioned this pull request Aug 1, 2023
2 tasks
The struct methods have been rearranged, to avoid increasing too much
the complexity, in particular for no special purpose
@alecandido
Copy link
Member Author

I'm completing the tests and the review of the runner API. The rest should be already alright (provided that tests are still passing).

To make @felixhekhorn happier: I dropped open(mode="..."), now we only have read/write (and edit, which is now just an alias for read(..., readonly=False)). But the Builder is still there, it's still useful :P

@alecandido
Copy link
Member Author

Since I'm reviewing the runner API: may I drop the legacy runner?

@felixhekhorn @giacomomagni @niclaurenti @andreab1997

@alecandido alecandido marked this pull request as ready for review August 17, 2023 14:42
@alecandido
Copy link
Member Author

alecandido commented Aug 17, 2023

I completed the review of the eko.runner API as well, now everything is up for discussion.

But before merging I'd like to have a plan for the legacy runner removal. This whole PR was meant to provide tools for Pineko to implement a further runner, similar to the managed one, but more sophisticated. There should be no reason to keep the legacy any further.

EDIT: now discussed #398

Copy link
Contributor

@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.

Looks fine. However, since this is surely part of the v0.14 series we should start merging the related PRs (order to be determined), since we have now several PRs touching these files (e.g. #291 )

@alecandido
Copy link
Member Author

Looks fine. However, since this is surely part of the v0.14 series we should start merging the related PRs (order to be determined), since we have now several PRs touching these files (e.g. #291 )

I was already on it

@felixhekhorn felixhekhorn mentioned this pull request Aug 17, 2023
@felixhekhorn felixhekhorn merged commit bd70e21 into master Aug 9, 2024
8 checks passed
@felixhekhorn felixhekhorn deleted the load-opened branch August 9, 2024 10:54
@felixhekhorn felixhekhorn mentioned this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request output Output format and management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants