Skip to content

don't clean up imported modules after verifying imports of included Python modules #3544

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

Merged
merged 2 commits into from
Jan 12, 2021

Conversation

boegel
Copy link
Member

@boegel boegel commented Jan 10, 2021

fixes #3542

…ython modules (fixes easybuilders#3542),

and make necessary changes in related tests so they keep passing
@boegel boegel added the bug fix label Jan 10, 2021
@boegel boegel added this to the next release (4.3.3?) milestone Jan 10, 2021
@ocaisa
Copy link
Member

ocaisa commented Jan 11, 2021

@Flamefire This looks ok to me since the clear() and update() are only being done in the tests, so we shouldn't run into these runtime issues any more. I don't trust that I fully understand the issue though, can you review this PR?

@Flamefire
Copy link
Contributor

I have found https://stackoverflow.com/questions/43181440/what-does-del-sys-modulesmodule-actually-do which references the python docs:

[sys.modules] is a dictionary that maps module names to modules which have already been loaded. This can be manipulated to force reloading of modules and other tricks. However, replacing the dictionary will not necessarily work as expected and deleting essential items from the dictionary may cause Python to fail.

So IMO even though it is test code this shouldn't be done as it seems to be unreliable (as shown by the issue: adding an extra import seems to have triggered the GC causing the issue to become a bug).
Why was the test code changed? Wasn't the old stuff (del and reload) enough?

I assume the del to be fine, so the module code change is good as far as I can tell, thanks!

@boegel
Copy link
Member Author

boegel commented Jan 11, 2021

I added the sys.modules.clear() in the test at some point because it seemed necessary, but it actually isn't, the del sys.modules[*] statements indeed work just as well; fixed in 482860a .

@boegel boegel force-pushed the no_sys_modules_cleanup branch from 9857893 to 482860a Compare January 11, 2021 20:17
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Occasional failures with Python 2, triggered by cleanup of imported modules in verify_imports
3 participants