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

Fix finalizer dependency on global experiment_runner #346

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Mar 12, 2024

The current finalizer in futures_util.py assumes that experiment_runner exists in the namespace of the module (i.e., .py file). However, this is only available in each of the separate futures models (i.e., multiprocessing, mpi). This fixes the resulting bug.

fixes a bug in the finalizer in case of working directory models
@quaquel quaquel added the bug label Mar 12, 2024
@quaquel quaquel added this to the 3.0 milestone Mar 12, 2024
@coveralls
Copy link

coveralls commented Mar 12, 2024

Coverage Status

coverage: 80.287% (-0.01%) from 80.3%
when pulling 6f7a1b2 on finalizer
into ebc96e9 on master.

Copy link
Collaborator

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Sounds good!

@EwoutH EwoutH changed the title bugfix in finalizer Fix finalizer dependency on global experiment_runner Mar 12, 2024
@EwoutH
Copy link
Collaborator

EwoutH commented Mar 12, 2024

Would it be useful to have some more test coverage here?

@quaquel
Copy link
Owner Author

quaquel commented Mar 12, 2024

Would it be useful to have some more test coverage here?

Yes, I want to still add a test that reproduces the original error. I just needed the fix last week for the pandemic example I was using.

@quaquel quaquel merged commit 10429e4 into master Mar 20, 2024
11 of 12 checks passed
@quaquel quaquel deleted the finalizer branch March 20, 2024 12:25
EwoutH pushed a commit that referenced this pull request Apr 17, 2024
* bugfix to finalizer

fixes a bug in the finalizer in case of working directory models

* Update futures_util.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants