Skip to content
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

Merged
merged 7 commits into from
May 19, 2024

Conversation

jo-basevi
Copy link
Collaborator

@jo-basevi jo-basevi commented May 3, 2024

  • 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

Modules are still loaded the same way in experiment.run so to not change users env vars when running payu setup

Closes #379

- 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
@pep8speaks
Copy link

pep8speaks commented May 3, 2024

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
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a 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.

payu/models/model.py Show resolved Hide resolved
payu/models/model.py Outdated Show resolved Hide resolved
payu/models/model.py Outdated Show resolved Hide resolved
payu/models/fms.py Outdated Show resolved Hide resolved
payu/envmod.py Outdated Show resolved Hide resolved
- 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
@jo-basevi
Copy link
Collaborator Author

Hi Aidan, thanks for the review! I've changed the logic a little bit to actually load user modules as part of payu setup rather than payu run(other modules will still get loaded in payu run).

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 payu run. There could be a confusing case where one modulefile was loaded because module use was used, but that was different to the module loaded only using module: use: in config.yaml`. Hopefully this case be caught with the above check.

@jo-basevi jo-basevi marked this pull request as ready for review May 8, 2024 00:25
aidanheerdegen
aidanheerdegen previously approved these changes May 9, 2024
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a 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 ...

payu/envmod.py Show resolved Hide resolved
payu/envmod.py Outdated Show resolved Hide resolved
payu/models/model.py Outdated Show resolved Hide resolved
payu/models/model.py Show resolved Hide resolved
payu/envmod.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a 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?

payu/models/model.py Show resolved Hide resolved
@aidanheerdegen aidanheerdegen self-requested a review May 16, 2024 02:19
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get model executables from path
3 participants