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

WIP: Concurrent loading of ensemble #77

Closed
wants to merge 24 commits into from

Conversation

wouterjdb
Copy link

Work towards addressing #11

@wouterjdb wouterjdb added the enhancement New feature or request label Dec 10, 2019
src/fmu/ensemble/ensemble.py Outdated Show resolved Hide resolved
src/fmu/ensemble/ensemble.py Outdated Show resolved Hide resolved
@berland
Copy link
Collaborator

berland commented Dec 10, 2019

Good start. Next up for grabs should be ScratchEnsemble.load_file() which is where there is really potential for speedup.

@wouterjdb
Copy link
Author

Good start. Next up for grabs should be ScratchEnsemble.load_file() which is where there is really potential for speedup.

@berland, something like this?

@wouterjdb
Copy link
Author

Managed to get a few tests further now.... now I got a segmentation error 😱 when running the test_ensemble_aggregations...... to be continued...

@wouterjdb
Copy link
Author

@berland OK, first working version of the code. We already had a chat about some of the downsides of the code as it is per today, but maybe you can have a look and identify what really needs to be done before moving towards a merge.

Seems to work identically in this way
Copy link
Author

@wouterjdb wouterjdb left a comment

Choose a reason for hiding this comment

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

This looks like a more elegant way of writing it - getting rid of all the boilerplate code! 👍

There is another use of concurrent in the same file - did you consider the same approach there? Or was that any different?


for realfuture in realfutures:
count += self._check_loading_of_realization(
realfuture.result(), "dummydirfornow", None)
Copy link
Author

Choose a reason for hiding this comment

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

Why did you use dummy arguments?

Copy link
Author

Choose a reason for hiding this comment

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

I updated those lines in the meanwhile by the way - let me know if that was not what you had in mind.

And I see that for the other usage there is not this additional wrapper (there is one, but it is different.). Though, we might consider changing that to the submit() method as well - just to use the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dummydir was just for laziness. Need to find a pretty solution, but it is only used for logging.

@berland berland mentioned this pull request Dec 12, 2019
@berland
Copy link
Collaborator

berland commented Dec 12, 2019

I had a look at how to remove the _load_smry() wrapper as well, but that is not that easy. But more importantly, it uncovers a flaw in the usage pattern - this way of working, calling load_ functions from the Ensemble object to update the realization object does not scale. Check the comment in #11

@berland berland changed the title Concurrent loading of ensemble WIP: Concurrent loading of ensemble Mar 24, 2020
@berland berland added the duplicate This issue or pull request already exists label Apr 16, 2020
@berland
Copy link
Collaborator

berland commented Apr 16, 2020

This PR is replaced by #106

@berland berland closed this Apr 16, 2020
@berland berland deleted the concurrentensembleloading branch April 30, 2020 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants