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

Reorder cleanup in repeat_report() #73

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

steipatr
Copy link
Contributor

Change order in which temporary files, handles, and folders are cleaned up in repeat_report(). If accepted, this closes #72 .

Change order in which temporary files, handles, and folders are cleaned up in repeat_report(). This fixes an error with multiprocessing and temporary files.
@EwoutH
Copy link
Contributor

EwoutH commented Jun 14, 2023

@steipatr Thanks for opening this PR! If you provide me with instructions how to test / reproduce I can test on Windows.

@quaquel I might have someone also experiencing this error. Could you review this PR and merge it if you approve? Then we could issue a patch release.

@quaquel quaquel merged commit c2c910e into quaquel:master Jun 14, 2023
@EwoutH
Copy link
Contributor

EwoutH commented Jun 14, 2023

Thanks for the very quick response, thanks!

Did we (or the CI) test this on a Unix / Linux system?

At some point using rmtree was discussed in #72. What was the consideration for going with this solution?

A potential rmtree looks quite clean:

import shutil

def repeat_report(self, netlogo_reporter, reps, go="go", include_t0=True):
    # ... existing code here ...

    # cleanup temp files and folders
    for fh in fhs:
        os.close(fh)  # free up file handle for re-use

    shutil.rmtree(tempfolder)  # remove folder and its contents

    return results

@quaquel
Copy link
Owner

quaquel commented Jun 14, 2023

I did not test it, nor do I have time atm to test it.

@EwoutH
Copy link
Contributor

EwoutH commented Jun 15, 2023

Anne confirmed that this solved the problem for her!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experiments work on machine with older env, but not another with newer env
3 participants