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

Reorganization of evaluator code and renaming of modules #320

Merged
merged 11 commits into from
Dec 8, 2023

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Dec 1, 2023

In preparation for future updates to the mpi code, all evaluator code has been reorganized. All evaluators go into their respective modules, and code shared among 2 or more goes into a new util package.

tests and docs have been updated as well to reflect these changes.

in preparation for future updates to the mpi code, all evaluator code has been reorganized. All evaluators go into their respective module, and code sthared among 2 or more goes into a new util package.

All relevant modules also have been renamed to future_{style of parallelization}
@quaquel quaquel added this to the 2.5.0 milestone Dec 1, 2023
@quaquel quaquel requested a review from EwoutH December 1, 2023 15:41
@coveralls
Copy link

coveralls commented Dec 1, 2023

Coverage Status

coverage: 80.332% (+0.08%) from 80.251%
when pulling c59ce16 on reorganization
into 2ae0278 on master.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 1, 2023

I will try to give it a look monday!

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 4, 2023

I have a few questions:

  • What was the initial motivation behind this?
  • What's the idea of moving everything to files with a futures_ prefix?
  • Why is ipyparallel now a docs requirement?

@quaquel
Copy link
Owner Author

quaquel commented Dec 4, 2023

I am, in parallel, working on expanding on the MPI code. I noticed that I could reuse parts of the multiprocessing code in this. However, it also gave rise to circular imports. To resolve these, at a minimum, I needed to move all reused parts into a separate util module. Moreover, for convenience, I prefer to have related functionality in the same module and keep the modules rather short. Therefore, I moved all evaluators to their respective modules (i.e., multiprocessing, mpi, ipyparallel). The net result is a code base that is more cleanly separated, easier to test in isolation, and prepares for further updates to mpi and multiprocressing.

The prefix was ema, but that is not informative. I went with futures to indicate it has to do with parallization, but I am open to other suggestions.

The reorganization resulted in import problems that caused the docs to fail. I fixed it quickly by making ipyparallel required. I looked at other solutions, but because I am subclassing stuff from ipyparallel, I could not use the method level imports as used with mpi4py.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 4, 2023

Thanks for the explanation.

Why do we need a prefix at all?

Edit: These are breaking changes right? As in it break current imports?

@quaquel
Copy link
Owner Author

quaquel commented Dec 4, 2023

If you don't, you can get namespace classes (e.g., em_framework.multiprocessing and multiprocessing).

No these are not breaking changes, because all relevant imports were available at the ema_workbench level already. It is only an internal reorganization.

@quaquel quaquel merged commit 2b1ad48 into master Dec 8, 2023
22 checks passed
@quaquel quaquel deleted the reorganization branch December 8, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants