-
Notifications
You must be signed in to change notification settings - Fork 20
modules: warn for modules:default:roots:tcl set and arch_folder:false
#276
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
Conversation
msimberg
left a comment
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.
Looks good to me.
I don't know if it might be worth expanding the messages to say something about arch_folder:true being the default? If a user leaves out arch_folder they might be confused why there's a complaint about arch_folder being enabled?
This is quite a corner case though and I think this is already very good.
|
In fact, is this worth mentioning in the docs as well? |
|
@msimberg you raise a very good point, on which I fully agree. The point is that we just use the spack schema (and related doc), since we merely proxy it to spack with the only change of the root folder (actually, we might think of adding at least a warning for that too, because we overwrite So, what we'd like to do is to constrain the functionalities, mainly:
One option might be to add this "extra" schema constraints as python checks on |
…of a separate setter for modulepath
773f83d to
fa0ab92
Compare
arch_folder:falsemodules:default:roots:tcl to be unset and arch_folder:false
|
@bcumming @msimberg what do you think of the current solution? I added a very basic schema for
If this solution is accepted, I will work on refining a couple of things, like the doc and the type of exception raised. I can also argument about a couple of design decisions. |
|
FYI, I've just checked in |
msimberg
left a comment
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.
Still looks good to me. One clarifying question.
|
I updated also the doc to help you understanding what's going on. If we agree on these changes, I'm going to open a PR in
Note: I'm not enforcing anything on |
move test-case from not-accepted to accepted, update doc
|
As per discussion about I'd still go for cleaning up recipes with the aforementioned branch, just to try steering future recipes (in case of copy-paste) in the new direction. EDIT: Just opened eth-cscs/alps-uenv#276 |
| # Note: | ||
| # modules root should match MODULEPATH set by envvars and used by uenv view "modules" | ||
| # so we enforce that the user does not override it in modules.yaml | ||
| self.modules["modules"].setdefault("default", {}).setdefault("roots", {}).setdefault( |
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.
this is a more robust solution 👍
modules:default:roots:tcl to be unset and arch_folder:falsemodules:default:roots:tcl set and arch_folder:false
This is a minor bug fix, since it does not apply to any recipe currently published in https://github.com/eth-cscs/alps-uenv, but it represents a good sanity check that might help users.
Since the default configuration for spack would be to consider
arch_folder: true, if the recipes don't explicitly set it, the modules get generated with an additional folder level, namely the arch_folder (e.g./user-environment/modules/linux-sles15-neoverse_v2instead of simply/user-environment/modules).The problem arises with usage, because
uenvsetsMODULEPATHto/user-environment/modules, so the modules are not anymore reachable simply by name, but they need to be prefixed with the specific arch_folder (e.g.linux-sles15-neoverse_v2/cmakeinstead ofcmake). That does not translates just to a burden for the user that has to prefix all module names with the arch_folder, but it actually does not work because modules dependencies in modules themselves are not prefixed with thearch_folder.Spack implements this functionality by adding to
MODULEPATHall arch_folders available on the system.I think we can live without functionality, but we should anyway enforce it to catch earlier (at build time) potential problems (which would otherwise show up just at usage time).
EDIT: they are not strictly related, but since they act on the same portion of code, I decided to introduce dependency on #275
modules.yaml