-
Notifications
You must be signed in to change notification settings - Fork 26
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
Get module executables from path loaded by environment modules #439
Get module executables from path loaded by environment modules #439
Conversation
- Move setting executable paths in Model class to Experiment.setup() - Inspect user modules (specified in config.yaml) to changes to PATH - The modules are restricted to user module directories defined in config.yaml - If exe is not an absolute path, search along above paths
Hello @jo-basevi! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-05-14 05:21:31 UTC |
- Refactor experiment module and model expand executable path functions - Run module setup as part of collate - Ignore module paths when setting executable path in model.build
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.
Some comments, but looks great, thanks.
- Loading user modules and modulepaths in experiment.setup to check if multiple modules are available - In collate, only load user modules so load_modules() is run, only when mpi modules are needed - Changed inspecting path over all modulepaths (including the ones added using `module use`) to reflect the module that will actually be loaded
- When searching paths for executable, check if file is executable
Hi Aidan, thanks for the review! I've changed the logic a little bit to actually load user modules as part of The idea was to actually check if the module will be found, and check if more than one module is available. It errors out if more than one module is available - this may be a breaking change for configs thats haven't added a version for modules? I am not sure if there exists a case when the same module and version exists in two modulepaths, but still want one of them to be loaded? If so, can change it to an warning.. I wanted to also load the actual modules that will be loaded as part of |
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, but as usual questions questions questions ...
…fore/after loading modules
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.
Were you planning on adding tests?
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, tests are probably too complicated for the return, so LGTM!
Modules are still loaded the same way in experiment.run so to not change users env vars when running
payu setup
Closes #379