-
Notifications
You must be signed in to change notification settings - Fork 51
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
Workaround for LMOD only allowing a single hook to be registered, which made our first hook unused #490
Workaround for LMOD only allowing a single hook to be registered, which made our first hook unused #490
Conversation
…t we specify in our RC file, see search order on https://lmod.readthedocs.io/en/latest/145_properties.html?highlight=lmod_rc#the-properties-file-lmodrc-lua . Also, prefix our hooks with eessi_ to avoid unintentional clashes in namespace between site lmod configuration and eessi lmod configuration
Instance
|
…a hook. Lmod only permits the registration of a single function, trying to register a second function will just overwrite the first, as can be seen from https://github.com/TACC/Lmod/blob/a97d68193a6f32ebca4b4bd70dac0f6ac8fddefe/src/Hook.lua#L87
Let me test first if I can indeed extend this hook... if extending it doesn't work anyway, the |
I can extend this hook. A solution that works is to have in the
And then in the site-specific
(N.B. this is a minimal code example, in reality you'd want to do checking if While this works and is a solution we can use already today, I've also opened a discussion upstream to see if |
Updating the Lmod version is going to take a while, I suggest we make this available now as it helps record what the necessary steps are on top of anything else. It's no longer WIP, is it? |
You're right, it isn't. I wanted to test it first, but my message from 3 hours ago was the conclusion that that works :D so, this is ready for review. |
One thing I'm wondering: should we set This way, I can just put a local |
To answer my own question: yes, I think we should do this, and I added it in the commit. Our docs should then just say "If you want to customize lmod behaviour for the EESSI environment, place an |
init/eessi_environment_variables
Outdated
show_msg "Found Lmod configuration file at $LMOD_RC" | ||
else | ||
error "Lmod configuration file not found at $LMOD_RC" | ||
fi | ||
# Allow for site-specific lmod configuration | ||
host_lmod_rc="$EESSI_CVMFS_REPO"/host_injections/.lmod/lmodrc.lua |
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 like the idea in principle, but I'd make it sit within a host_injections equivalent of LMOD_CONFIG_DIR
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.
You mean: nested, so that you have e.g. a /cvmfs/software.eessi.io/host_injections/2023.06/software/linux/x86_64/amd/zen2/.lmod/lmodrc.lua
?
We could. But do you really need to be able to make architecture-specific hooks? In EESSI itself, we only make one hook file with create_lmodrc.py
and use logic to do architecture specific things like if (moduleName == "OpenMPI") and (cpuTarget == "aarch64/neoverse_v1") then ...
. Then we just duplicate the lmodrc.lua
to all prefixes
The downside of having this nested in an architecture prefix is that you might need to do a lot of duplication (or at the very least symlinking) as a host site if you have a heterogenous system. For the Snellius system at SURF, it'd already mean putting the same lmodrc.lua
in two different prefixes today, and if we have a zen4
tree tomorrow, I'll also need to remember to deploy it there...
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.
On an arch specific level perhaps not, but at least we should use ${EESSI_PREFIX/versions/host_injections}
(i.e., /cvmfs/software.eessi.io/host_injections/2023.06
)
I'm working on the upstream issue of allowing the registration of multiple hooks of the same type by putting them into a table (instead of overwriting). In the process, I have installed a newer version of
It seems the load hook is now only called if for modules with the GPU properties assigned. I'm wondering if this may be the result of a change in Lmod of when the The documented place to specify hooks is I'll confirm if the version is the issue by installing different versions of lmod and seeing if I get the original behaviour back... |
Ok, I'm stumped. I installed the
Running with that installation, I still only see the modules with a GPU property invoking the hook:
Why? What's different? I don't know... |
Inspecting the debugging output with:
I find:
I.e. it is indeed the
Which indeed happens very early in the trace. It is part of initializing the |
FYI: I made two PRs with two possible approaches for supporting registration of more than one function as a hook: |
…tware.eessi.io/host_injections/2023.06/.lmod/lmodrc.lua
We'll have to make the switch to using |
bot: build repo:eessi.io-2023.06-software arch:x86_64/amd/zen3 |
Updates by the bot instance
|
New job on instance
|
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.
Meant to request a change in last review
@casparvl The changes to the initialisation script are not being picked up (according to #490 (comment)). I'm guessing this is deliberate behaviour of our tarring script...but I don't know how to deal with it. |
@casparvl Thinking about this again, I would revert all your changes to |
Also true, ok, I'm stripping that and limiting it to the actual fix. |
…ug fix only. We can follow up properly using SitePackage.lua instead
bot: build repo:eessi.io-2023.06-software |
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
All staging PRs merged |
Use LMOD_CONFIG_DIR instead of LMOD_RC to allow user overrides of what we specify in our RC file, see search order on https://lmod.readthedocs.io/en/latest/145_properties.html?highlight=lmod_rc#the-properties-file-lmodrc-lua . Also, prefix our hooks with eessi_ to avoid unintentional clashes in namespace between site lmod configuration and eessi lmod configuration