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

Update check for load user modulefiles to allow exact match #482

Merged

Conversation

jo-basevi
Copy link
Collaborator

Allow checks for "modules: load:" modulefiles to allow when there's an exact match in available modules. This is to fix bug #481 where module avail for openmpi/4.1.5 includes openmpi/4.1.5 and openmpi/4.1.5-debug.

I've also added tests for parsing module avail output.

Closes #481.

@aidanheerdegen last week there was an issue with multiple modules found in /g/data/vk83/modules and /g/data/vk83/modules/access-models, even though they were the same modulefile. I noticed in the modules docs there's a module paths command that lists paths of available modulefiles matching a pattern so for the above, it would still evaluate to the same path. There's a couple of differences to module avail:

  • Running the command module paths using modulecmd ($MODULESHOME//bin/modulecmd) generates a bunch of code. So for bash, it generates a bunch of echo statements, rather than output going to stderr which is the case for module avail. So to get what values are printed, have to run the generated code, and then capture the output.
  • Specifying openmpi/ with a / just matches the default module version (I think), so only one path is returned.

If it's unlikely to have a case where module/version can be accessed from two different modulepaths and are different, e.g.

/path/to/modulefiles:
test-module/1.0.0
/another/module/path:
test-module/1.0.0

The check could be changed from modules_avail.count(modulefile) != 1 to modulefile not in modules_avail in this PR. Maybe this be moved to separate to avoid blocking this PR?

@pep8speaks
Copy link

pep8speaks commented Aug 13, 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-08-13 23:31:54 UTC

This is to fix bug payu-org#481 where module avail for `openmpi/4.1.5` includes `openmpi/4.1.5` and `openmpi/4.1.5-debug`
Added tests for parsing module avail output
@jo-basevi jo-basevi force-pushed the 481-fix-payu-module-uniqueness-check branch from 392ff77 to b69a0db Compare August 13, 2024 04:44
@coveralls
Copy link

coveralls commented Aug 13, 2024

Coverage Status

coverage: 52.612% (+0.3%) from 52.281%
when pulling bfecc12 on ACCESS-NRI:481-fix-payu-module-uniqueness-check
into 83001a2 on payu-org:master.

@jo-basevi jo-basevi marked this pull request as ready for review August 13, 2024 04:46
aidanheerdegen
aidanheerdegen previously approved these changes Aug 13, 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.

Some comments ... but happy to merge if they're ignorable.

test/test_envmod.py Show resolved Hide resolved
test/test_envmod.py Show resolved Hide resolved
@aidanheerdegen
Copy link
Collaborator

last week there was an issue with multiple modules found in /g/data/vk83/modules and /g/data/vk83/modules/access-models, even though they were the same modulefile

That was an implementation bug. We've removed the access-models subdirectory to fix it. So it isn't a use case we need to care about or support. Does that help?

@jo-basevi
Copy link
Collaborator Author

Thanks for the review!

That was an implementation bug. We've removed the access-models subdirectory to fix it. So it isn't a use case we need to care about or support. Does that help?

Yes, that does help thanks. Ok so don't have to worry about using module paths or changing the implementation in that case.

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.

LGTM.

test/test_envmod.py Show resolved Hide resolved
@jo-basevi jo-basevi merged commit e611bcc into payu-org:master Aug 14, 2024
9 checks passed
@jo-basevi jo-basevi deleted the 481-fix-payu-module-uniqueness-check branch August 14, 2024 02:04
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.

Payu's check for module uniqueness fails incorrectly
4 participants