Skip to content

Fix finalizer dependency on global experiment_runner#346

Merged
quaquel merged 3 commits intomasterfrom
finalizer
Mar 20, 2024
Merged

Fix finalizer dependency on global experiment_runner#346
quaquel merged 3 commits intomasterfrom
finalizer

Conversation

@quaquel
Copy link
Copy Markdown
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.

quaquel added 2 commits March 11, 2024 19:56
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Collaborator

EwoutH commented Mar 12, 2024

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

@quaquel
Copy link
Copy Markdown
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
@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