fix verify_imports by deleting all imported modules before re-importing them one by one

[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)

[migueldiascosta]

@Flamefire the python2 vs python3 part was bothering me, and using a very simple local test looking at the id of the objects I now think this also fixes a more insidious bug when using python3, because in that case it seems the original class object was still somehow being used, even though the module was removed from sys.modules and the re-imported class object is not the same

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

[Flamefire]

this also fixes a more insidious bug when using python3, because in that case it seems the original class object was still somehow being used

That's what I feared and mentioned in the confcall. :/ Thanks for checking and verifying that this now works!

[migueldiascosta]

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?

[Flamefire]

but maybe this should go in a different PR?

I think it fits in here. I also added 2 minor fixes that I noticed while doing this, see the commit list.
Note that I removed the skipping of the test you suggested to modify when no github token is available. --include-easyblocks-from-pr should also work without a token. Kept it at the other tests though because IIRC there is some rate limiting issue. As this is the most tricky test I think it is good enough to test this one.

[migueldiascosta]

@Flamefire I can reproduce the error in test_github_add_pr_labels with python2, it happens when run after test_github_xxx_include_easyblocks_from_pr after we added the include from PR 2479...

[Flamefire]

@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...

[migueldiascosta]

@Flamefire test_github_xxx_include_easyblocks_from_pr doesn't fail for me, it's test_add_pr_labels that fails but only when run after test_github_xxx_include_easyblocks_from_pr, and only in python2, and only when the include test also includes easyblocks from pr 2479. But nothing I do to "clean" sys.modules seems to fix it...

[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...

[migueldiascosta]

However added a fix for that: We need to remove the newly imported configuremake easyblock

yeah, but it's not enough (just tried again on top of your commit 9237b9f, just to be sure)

[migueldiascosta]

And the test_github_xxx_include_easyblocks_from_pr failed on CI, no idea why. Let's see...

Indeed. That one I haven't been able to reproduce locally yet...

[Flamefire]

Indeed. That one I haven't been able to reproduce locally yet...

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.

[Flamefire]

yeah, but it's not enough (just tried again on top of your commit 9237b9f, just to be sure)

@migueldiascosta Can you try again? If it still fails, what is the error?

[migueldiascosta]

@migueldiascosta Can you try again? If it still fails, what is the error?

still the same...

$ git log -n 2
commit aa40a7bed142d3ae9bd10910e79c54c45dd40b50
Author: Alexander Grund <alexander.grund@tu-dresden.de>
Date:   Thu Jul 29 13:15:58 2021 +0200

    Allow custom test token for github tests
    
    Use EB_GITHUB_TEST_ACCOUNT env variable to specify your personal account
    to use for tests.
    Useful to avoid rate limits

commit 289d57523f5b8a2171a98790dd609cad01822447
Author: Alexander Grund <alexander.grund@tu-dresden.de>
Date:   Thu Jul 29 13:15:44 2021 +0200

    Fix import order of easyblocks
$ python2 -O -m test.framework.suite include_easyblocks_from_pr add_pr_labels
(...)
Filtered CommandLineOptionsTest tests using 'include_easyblocks_from_pr|add_pr_labels', retained 1/125 tests: test_github_xxx_include_easyblocks_from_pr
(...)
Filtered GithubTest tests using 'include_easyblocks_from_pr|add_pr_labels', retained 1/29 tests: test_github_add_pr_labels
..E
======================================================================
ERROR: test_github_add_pr_labels (test.framework.github.GithubTest)
Test add_pr_labels function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/framework/github.py", line 180, in test_github_add_pr_labels
    gh.add_pr_labels(8006)  # closed, unmerged, unlabeled PR
  File "easybuild/tools/github.py", line 1499, in add_pr_labels
    file_info = det_file_info(pr_files, download_repo_path)
  File "/home/apps/develop/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 2371, in det_file_info
    ecs = process_easyconfig(path, validate=False)
  File "/home/apps/develop/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 2018, in process_easyconfig
    ec = EasyConfig(spec, build_specs=build_specs, validate=validate, hidden=hidden)
  File "/home/apps/develop/easybuild-framework/easybuild/framework/easyconfig/easyconfig.py", line 481, in __init__
    self.extra_options = easyblock_class.extra_options()
  File "/home/apps/develop/easybuild-framework/test/framework/sandbox/easybuild/easyblocks/generic/makecp.py", line 49, in extra_options
    return ConfigureMake.extra_options(extra_vars=extra)
  File "/home/apps/develop/easybuild-framework/test/framework/sandbox/easybuild/easyblocks/generic/configuremake.py", line 41, in extra_options
    extra_vars = EasyBlock.extra_options(extra=extra_vars)
AttributeError: 'NoneType' object has no attribute 'extra_options'

[Flamefire]

@migueldiascosta found it: In Python2 reload does not update from foo import bar stuff, so that is why bar is undefined/None at that point. Same issue actually as with failing VERSION from the easyblocks module which I could workaround.
My current solution is to simply remove all loaded easyblock modules. This seems to force loading it which reruns the import
A less brutal solution would be to manually delete some easyblocks but I'm not sure how to find out which to delete. So yeah...

[Flamefire]

@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

[migueldiascosta]

lgtm

[migueldiascosta]

Going in, thanks @Flamefire!