allow including easyblocks from multiple locations #3311

migueldiascosta · 2020-04-30T08:38:52Z

(created using eb --new-pr)

boegel

lgtm

boegel · 2020-04-30T09:38:39Z

@zao Can you confirm that the problem you ran into with eb --from-pr 10513 --include-easyblocks-from-pr 2046 is fixed with these changes?

migueldiascosta · 2020-04-30T10:45:51Z

well, it won't fix @zao's problem, merely output a better error message

to fix it we would need to make a decision: should easyblocks included from a PR always take precedence over the others?

boegel · 2020-04-30T12:43:25Z

@migueldiascosta Oh, OK.

I'd say the easyblocks from the PR should get preference, otherwise what's the point of using it if local easyblocks can get the upper hand?

Would you agree @zao?

boegel · 2020-05-01T07:00:58Z

easybuild/tools/options.py

@@ -1432,7 +1433,8 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False):
        if eb_go.options.include_easyblocks:
            # make sure we're not including the same easyblock twice
            included_from_pr = set([os.path.basename(eb) for eb in easyblocks_from_pr])
-            included_from_file = set([os.path.basename(eb) for eb in eb_go.options.include_easyblocks])
+            included_paths = expand_glob_paths(eb_go.options.include_easyblocks)
+            included_from_file = set([os.path.basename(eb) for eb in included_paths])
            included_twice = included_from_pr & included_from_file
            if included_twice:
                raise EasyBuildError("Multiple inclusion of %s, check your --include-easyblocks options",


@migueldiascosta Rather than raising an error, I think we should let the easyblocks that are included from the PR win over any local easyblocks included with --include-easyblocks. That makes perfect sense to me, since I don't see a use case where you would be using --include-easyblocks-from-pr but you would still like to see local easyblocks get precedence?

It's probably also a good idea to print a message for the easyblocks that get pulled in from a PR, to make that a bit more visible?

== easyblock example.py included from PR #1234

Actually, the easyblocks included from the PR should already take precedence, because they are included later and their path is prepended.

I hadn't realised this before but when the local easyblocks are included, they are immediately imported in verify_imports and they remain in sys.modules. When the same easyblock is included later from a PR, verify_imports fails because it continues to use previously imported module.

Not sure if this would be the best approach, but at least to demonstrate what is happening, if we add sys.modules.pop(pymod_spec) at the end of the loop in verify_imports, then the error reported by @zao goes away, the easyblocks from a PR take precedence, and we can stop raising this error

akesandgren · 2020-05-18T11:16:04Z

@migueldiascosta Oh, OK.

I'd say the easyblocks from the PR should get preference, otherwise what's the point of using it if local easyblocks can get the upper hand?

Would you agree @zao?

I'd agree at least.

akesandgren · 2020-05-18T11:17:33Z

@migueldiascosta Need a conflict resolution now.

boegel · 2020-05-19T07:33:31Z

I moved this to the next milestone, it needs a bit of work (not much though I think), and I don't think this should block the v4.2.1 release.

akesandgren · 2020-08-25T06:09:20Z

@migueldiascosta still needs conflict resolution and according to @boegel some more work...

migueldiascosta · 2020-08-25T07:05:09Z

the actual problem that @zao reported, which occurs when the same easyblock is included both from --include-easyblocks (e.g. from a site configuration) and --include-easyblocks-from-pr, is caused by how verify_imports (in easybuild.tools.include) works - once it verifies an easyblock is imported from the expected location, the module remains loaded, so when verify_imports is run again for the same easyblock from a different location, the locations don't match and it fails

in order to let the easyblocks included from a PR win over any local easyblock, as @boegel suggested, I think we need to either

a) at the end of verify_imports, restore sys.modules to its original state, or

b) delay calling verify_imports until after all potential sources of easyblocks are parsed and a single location for each easyblock is selected

To me it seems that a) would be best because I hadn't anticipated this behaviour from verify_imports, but I may be missing something - thoughts?

migueldiascosta · 2020-08-26T03:10:47Z

otoh, if the goal of verify_imports is not only to verify the import at that time but precisely to prevent the module from being imported later from a different location, then I can do b) above or c) delay calling include_easyblocks altogether, so that it's called only once with the selected paths from both --include-easyblocks and --include-easyblocks-from-pr

boegel · 2020-08-27T07:09:53Z

@migueldiascosta I think simply cleaning up after doing the verification in verify_imports makes sense. Having the imported easyblock "cached" in sys.modules is a nasty side-effect of verify_imports, it's better to avoid that.

The intent of verify_imports is just to verify whether importing the Python modules that were added in include_easyblocks & friends can actually be imported at that time, it's not the intention to ensure nothing else can shadow them a bit later (since there's no way to make that impossible anyway, if anything fiddles with sys.modules after verify_imports was run there are no guarantees).

…at the one from the PR will be used

boegel · 2020-09-01T21:10:23Z

easybuild/tools/options.py

+                         ','.join(included_twice))
+
+        for easyblock in included_from_pr:
+            log.info("== easyblock %s included from PR #%s", easyblock, eb_go.options.include_easyblocks_from_pr)


@migueldiascosta Let's make this more verbose by using print_msg, so it's not just included in the log file but also shown in the output produced by eb?

print_msg("== easyblock %s included from PR #%s" % (easyblock, eb_go.options.include_easyblocks_from_pr)), log=self.log)

It's probably also worth introducing a local variable for eb_go.options.include_easyblocks_from_pr?

pr_easyblocks = eb_go.options.include_easyblocks_from_pr if pr_easyblocks: ...

boegel · 2020-09-01T21:11:26Z

easybuild/tools/options.py

            included_twice = included_from_pr & included_from_file
            if included_twice:
-                raise EasyBuildError("Multiple inclusion of %s, check your --include-easyblocks options",
-                                     ','.join(included_twice))
+                log.info("EasyBlock(s) %s are included from multiple locations, the one from a PR will be used",


Maybe making this print_warning makes sense, to avoid surprises?

…ltiple locations

boegel

@migueldiascosta fixed with migueldiascosta#14

easybuild/tools/options.py

mention PR id in warning message when easyblocks are included from multiple locations + catch output in test

expand glob paths when checking for multiple inclusion of easyblocks

971ac5d

migueldiascosta added the bug fix label Apr 30, 2020

migueldiascosta requested a review from boegel April 30, 2020 08:39

boegel previously approved these changes Apr 30, 2020

View reviewed changes

boegel added this to the next release (4.2.1?) milestone Apr 30, 2020

boegel reviewed May 1, 2020

View reviewed changes

boegel modified the milestones: 4.2.1, release after 4.2.1 (4.2.2?) May 19, 2020

boegel modified the milestones: 4.2.2, release after 4.2.2 (4.2.3?) Jul 3, 2020

migueldiascosta added 4 commits August 27, 2020 15:30

sync with develop

d925a64

restore sys.modules at the end of verify_imports

7a7b8a1

allow including the same easyblock from a file and from a PR, warn th…

60f71a3

…at the one from the PR will be used

update include_easyblocks tests

150d911

migueldiascosta dismissed boegel’s stale review via 150d911 August 27, 2020 14:46

migueldiascosta changed the title ~~expand glob paths when checking for multiple inclusion of easyblocks~~ allow including easyblocks from multiple locations Aug 27, 2020

boegel requested changes Sep 1, 2020

View reviewed changes

migueldiascosta and others added 3 commits September 2, 2020 11:28

make messages and warnings more explicit

37abe78

mention PR id in warning message when easyblocks are included from mu…

92c97dc

…ltiple locations

catch & check output produced by --include-easyblocks-from-pr in test

292b7ba

boegel mentioned this pull request Sep 2, 2020

mention PR id in warning message when easyblocks are included from multiple locations + catch output in test migueldiascosta/easybuild-framework#14

Merged

boegel requested changes Sep 2, 2020

View reviewed changes

easybuild/tools/options.py Outdated Show resolved Hide resolved

Merge pull request #14 from boegel/20200430163846_new_pr_cworcWSdYy

efe98ba

mention PR id in warning message when easyblocks are included from multiple locations + catch output in test

boegel approved these changes Sep 2, 2020

View reviewed changes

boegel merged commit 277961b into easybuilders:develop Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow including easyblocks from multiple locations #3311

allow including easyblocks from multiple locations #3311

migueldiascosta commented Apr 30, 2020

boegel left a comment

boegel commented Apr 30, 2020

migueldiascosta commented Apr 30, 2020

boegel commented Apr 30, 2020

boegel May 1, 2020 •

edited

Loading

migueldiascosta May 2, 2020

akesandgren commented May 18, 2020

akesandgren commented May 18, 2020

boegel commented May 19, 2020

akesandgren commented Aug 25, 2020

migueldiascosta commented Aug 25, 2020

migueldiascosta commented Aug 26, 2020

boegel commented Aug 27, 2020

boegel Sep 1, 2020

boegel Sep 1, 2020

boegel left a comment

allow including easyblocks from multiple locations #3311

allow including easyblocks from multiple locations #3311

Conversation

migueldiascosta commented Apr 30, 2020

boegel left a comment

Choose a reason for hiding this comment

boegel commented Apr 30, 2020

migueldiascosta commented Apr 30, 2020

boegel commented Apr 30, 2020

boegel May 1, 2020 • edited Loading

Choose a reason for hiding this comment

migueldiascosta May 2, 2020

Choose a reason for hiding this comment

akesandgren commented May 18, 2020

akesandgren commented May 18, 2020

boegel commented May 19, 2020

akesandgren commented Aug 25, 2020

migueldiascosta commented Aug 25, 2020

migueldiascosta commented Aug 26, 2020

boegel commented Aug 27, 2020

boegel Sep 1, 2020

Choose a reason for hiding this comment

boegel Sep 1, 2020

Choose a reason for hiding this comment

boegel left a comment

Choose a reason for hiding this comment

boegel May 1, 2020 •

edited

Loading