-
Notifications
You must be signed in to change notification settings - Fork 217
add support for multi_deps easyconfig parameter (syntactic sugar for iterative list of builddependencies) #2813
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
+329
−37
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
5751312
add support for multi_deps easyconfig paramter (syntactic sugar for i…
boegel a655336
also consider builddependencies to define template values like %(pyve…
boegel 4670776
move extensions step into list of iterative steps
boegel 97d884a
add post_iter step to restore after iterating (rather than doing that…
boegel 3e77c22
add support to pass extra modules to load in sanity_check_step
boegel 7c1d4a3
add postiter step to list of steps run when using --module-only
boegel 626e319
also run extensions step in first iteration (oopsie)
boegel aa8fd33
also restore easyconfig parameters over which was iterated for extens…
boegel 63372d7
make sanity check aware of multi_deps
boegel 6382776
move iterated sanity check for multi-deps into dedicated method _sani…
boegel f43136d
flesh out get_multi_deps method from _sanity_check_step_multi_deps, s…
boegel 59f47cd
make ExtensionEasyBlock.sanity_check_step aware of multi_deps
boegel cd2d06e
add trace/log message for loading extra modules for extension sanity …
boegel 35b41ee
only run iterative sanity check for extension in ExtensionEasyBlock f…
boegel c3cb4f5
print message when starting new iteration
boegel 18e0d6e
move EasyBlock.get_multi_deps to EasyConfig.get_parsed_multi_deps + e…
boegel 23b28da
Merge branch 'develop' into multi_deps
boegel 85f17b6
only run multi-deps sanity check for non-extensions
boegel 3c34722
fix remarks + test for incorrect multi_deps spec with different # dep…
boegel 6d8e369
avoid keeping multiple copies of each extension in self.ext_instances…
boegel 62afc42
Delete rogue comment.
bartoldeman 219849d
Fix dumping of iterated builddependencies (regression from fcdc90)
bartoldeman 77d1d84
Fix dump of easyconfig with multi_deps for reprod.
bartoldeman e8ecb32
Merge pull request #43 from ComputeCanada/multi_deps_dump_fixes
boegel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that this only makes for the case of build_deps but I still think that there's no harm in making that connection explicit here (for those who haven't thought so deeply about it), how about
multi_builddeps?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that could make sense, but it's kind of double...
What we're doing is installing something for multiple versions of a particular dependency (e.g.
Python) in a single installation prefix.Those dependencies are generally runtime dependencies, but during the installation process we only use them as build dependencies, to avoid that they are included as a runtime dep in the generated module file.
That's the only way we can iterate over dependencies without introducing conflicts in the generated module file.
That this is done by injecting stuff into
builddependenciesdoesn't have to be explicit imho, it's kind of an implementation detail...One concern with the multi-Python installations (raised by @Micket) is that users will have to know to load a Python module as well for things to work.
This could be remedied by loading one of the
Pythonversions (e.g. the first listed inmulti_deps) by default, if no otherPythonversion is loaded already (perhaps with an accompanying message). The logical choice there would be the Python 3.x version. Note that this still leaves the door open to use anotherPythonversion, which is just amodule swapaway...I haven't made up my mind how to implement that yet (and it should probably be part of this PR)...
It could be done in general in the framework, or only for Python packages (but then we'll need to touch multiple generic easyblock like
PythonPackage,PythonBundle, etc.).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense.
Regarding letting users know a Python module is required is something that can be handled with
depends_on('Python')(for Lmod) without providing a version. That way the default is chosen by the site, without them doing something explicit it would be the latest version of Python 3. This could also work in general,depends_on('%(name)s'). Of course it would mean that if they had multiple subversions of Python the dep tree might change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ocaisa That won't work imho, since you'd be in trouble when a new
Python3.x module is installed with a different subtoolchain...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you go with your flat naming scheme ruining the party again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for loading a default module is not implemented yet in this PR, I'll do that in a follow-up PR to avoid making this PR too big/complicated...
It's a nice enhancement to have, but not a strict requirement to make this work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the
depends_onsupport in the future. One could have a guard, to check if the MNS is hierarchical or not, and usedepends_onselectively on those cases. But the guard might not be trivial to make it general for custom MNS