fix verify_imports by deleting all imported modules before re-importing them one by one #3780
Conversation
|
@Flamefire the python2 vs python3 part was bothering me, and using a very simple local test looking at the Deleting all modules before re-importing them fixes this, so even though I still don't understand completely what is happening in python3, there must be some sort of cache, and this PR invalidates that cache, which is what we want |
That's what I feared and mentioned in the confcall. :/ Thanks for checking and verifying that this now works! |
|
So, I wanted to suggest a test to make sure the easyblocks included from a PR do take preference even in the case when included easyblocks are related by inheritance, something like --- a/test/framework/options.py
+++ b/test/framework/options.py
@@ -3445,17 +3445,25 @@ class CommandLineOptionsTest(EnhancedTestCase):
# include test cmakemake easyblock
cmm_txt = '\n'.join([
- 'from easybuild.framework.easyblock import EasyBlock',
- 'class CMakeMake(EasyBlock):',
+ 'from easybuild.easyblocks.generic.configuremake import ConfigureMake',
+ 'class CMakeMake(ConfigureMake):',
' pass',
''
])
write_file(os.path.join(self.test_prefix, 'cmakemake.py'), cmm_txt)
- # including the same easyblock twice should work and give priority to the one from the PR
+ # include extra (wrong) configuremake easyblock to check that the one from a pr is preferred
+ configuremake_txt = '\n'.join([
+ 'class ConfigureMake(object):', # using object here instead of EasyBlock would make get_easyblock_class fail
+ ' pass',
+ ''
+ ])
+ write_file(os.path.join(self.test_prefix, 'configuremake.py'), configuremake_txt)
+
+ # including the same easyblock(s) twice should work and give priority to the one(s) from PR(s)
args = [
'--include-easyblocks=%s/*.py' % self.test_prefix,
- '--include-easyblocks-from-pr=1915',
+ '--include-easyblocks-from-pr=1915,2479',
'--list-easyblocks=detailed',
'--unittest-file=%s' % self.logfile,
'--github-user=%s' % GITHUB_TEST_ACCOUNT,
@@ -3468,10 +3476,18 @@ class CommandLineOptionsTest(EnhancedTestCase):
self.mock_stdout(False)
logtxt = read_file(self.logfile)
- expected = "WARNING: One or more easyblocks included from multiple locations: "
- expected += "cmakemake.py (the one(s) from PR #1915 will be used)"
- self.assertEqual(stderr.strip(), expected)
- self.assertEqual(stdout, "== easyblock cmakemake.py included from PR #1915\n")
+ expected = "\nWARNING: One or more easyblocks included from multiple locations: "
+ expected += "cmakemake.py (the one(s) from PR #1915 will be used)\n\n"
+ expected += "\nWARNING: One or more easyblocks included from multiple locations: "
+ expected += "configuremake.py (the one(s) from PR #2479 will be used)\n\n"
+ self.assertEqual(stderr, expected)
+ expected = "== easyblock cmakemake.py included from PR #1915\n"
+ expected += "== easyblock configuremake.py included from PR #2479\n"
+ self.assertEqual(stdout, expected)
+
+ # easyblock is found via get_easyblock_class
+ klass = get_easyblock_class('CMakeMake')
+ self.assertTrue(issubclass(klass, EasyBlock), "%s is an EasyBlock derivative class" % klass)
# easyblock included from pr is found
path_pattern = os.path.join(self.test_prefix, '.*', 'included-easyblocks-.*', 'easybuild', 'easyblocks')
@@ -3479,10 +3495,6 @@ class CommandLineOptionsTest(EnhancedTestCase):
cmm_regex = re.compile(r"\|-- CMakeMake \(easybuild.easyblocks.generic.cmakemake @ %s\)" % cmm_pattern, re.M)
self.assertTrue(cmm_regex.search(logtxt), "Pattern '%s' found in: %s" % (cmm_regex.pattern, logtxt))
- # easyblock is found via get_easyblock_class
- klass = get_easyblock_class('CMakeMake')
- self.assertTrue(issubclass(klass, EasyBlock), "%s is an EasyBlock derivative class" % klass)
-
# 'undo' import of easyblocks
del sys.modules['easybuild.easyblocks.foo']
del sys.modules['easybuild.easyblocks.generic.cmakemake']but it turns out it still fails even after the fix in this PR because of a separate bug, easyblocks from PRs are included one PR at a time... this would fix it --- a/easybuild/tools/options.py
+++ b/easybuild/tools/options.py
@@ -1510,6 +1510,7 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False):
included_paths = expand_glob_paths(eb_go.options.include_easyblocks)
included_from_file = set([os.path.basename(eb) for eb in included_paths])
+ easyblocks_from_all_prs = []
for easyblock_pr in easyblock_prs:
easyblocks_from_pr = fetch_easyblocks_from_pr(easyblock_pr)
included_from_pr = set([os.path.basename(eb) for eb in easyblocks_from_pr])
@@ -1525,7 +1526,9 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False):
for easyblock in included_from_pr:
print_msg("easyblock %s included from PR #%s" % (easyblock, easyblock_pr), log=log)
- include_easyblocks(eb_go.options.tmpdir, easyblocks_from_pr)
+ easyblocks_from_all_prs.extend(easyblocks_from_pr)
+
+ include_easyblocks(eb_go.options.tmpdir, easyblocks_from_all_prs)
if eb_go.options.list_easyblocks:
msg = list_easyblocks(eb_go.options.list_easyblocks, eb_go.options.output_format)but maybe this should go in a different PR? |
Avoids errors on on Python2 when we delete a subclass after importing a module with a class depending on it. Fixes easybuilders#3779
03a340f to
541c41f
Compare
I think it fits in here. I also added 2 minor fixes that I noticed while doing this, see the commit list. |
|
@Flamefire I can reproduce the error in |
|
@migueldiascosta Yeah if one of those sys.path modifying tests fails then the other will fail as well. But why does test_github_xxx_include_easyblocks_from_pr fail? It doesn't fail locally... |
|
@Flamefire |
|
@migueldiascosta Can't test that due to missing access rights I suppose. However added a fix for that: We need to remove the newly imported configuremake easyblock And the test_github_xxx_include_easyblocks_from_pr failed on CI, no idea why. Let's see... |
yeah, but it's not enough (just tried again on top of your commit 9237b9f, just to be sure) |
Indeed. That one I haven't been able to reproduce locally yet... |
4ae81be to
451a479
Compare
Found and fixed. We use the sandbox easyblocks and that doesn't have VERSION. Added that and fixed the import and sys.path changes so that a custom easyblocks package is not picked up anymore and the issue reproduces locally. |
@migueldiascosta Can you try again? If it still fails, what is the error? |
still the same... |
c72c679 to
10f5e28
Compare
|
@migueldiascosta found it: In Python2 |
10f5e28 to
08aa9ca
Compare
541c85b to
bf87587
Compare
bf87587 to
c61efb4
Compare
|
@migueldiascosta As fixing the other issue and the import things is hard I moved that to another PR so we can at least get this one in |
|
Going in, thanks @Flamefire! |
Avoids errors on on Python2 when we delete a subclass after importing
a module with a class depending on it.
Fixes #3779
Thanks @migueldiascosta who was on spot with #3779 (comment)