Avoid using a priority in prepend_module_path (Lmod) to avoid costly module calls #3636

Flamefire · 2021-04-07T10:40:02Z

This PR avoids using a priority in prepend_module_path (Lmod) which means that $MODULEPATH can be directly modified by this and all subsequent calls to either prepend_module_path or use which is a lot faster than calling Lmod.

The test has been changed so it now expects that prepend_module_path actually does prepend even when no priority is passed and another priority is in effect.
For compatibility I kept the constant 10000 as HIGH_PRIORITY so user sites that really want other module paths to take preference can use any higher priority.

As discussed in Slack this change is unlikely to effectively change any behavior and will very likely continue to work, see TACC/Lmod#509 (comment)

Fixes #3631

boegel · 2021-04-07T12:17:53Z

@Flamefire Let's close #3634 and continue the discussion here. When this PR (#3636) gets merged, #3634 will also be merged automatically (since the commits included in here are also included in there), but context w.r.t. the discussion will be missing...

easybuild/tools/modules.py

boegel · 2021-04-07T12:21:18Z

easybuild/tools/modules.py

+                priority = self.HIGH_PRIORITY
            self.use(path, priority=priority)
+            modulepath = curr_module_paths(clean=False)
+            path_idx = modulepath.index(path)


This could trigger a ValueError if (somehow...) path is not in $MODULEPATH, so maybe wrap this in a try/except to avoid a nasty traceback?

It would be really weird to not have path at all in modulepath though, I must admit.

It should be virtually impossible for this to happen as it should be either caught already by our tests or is an actual (and serious) bug in Lmod. So I'd say the "nasty traceback" is fine in this rare case so we don't need to litter our code :)

boegel · 2021-04-07T12:23:55Z

easybuild/tools/modules.py



-def curr_module_paths():
+def curr_module_paths(clean=True):


I wouldn't make cleaning optional at all, I think weird things may happen when empty values are included in $MODULEPATH and EasyBuild starts using those. There's a reason we're filtering those here...

This is required to match Lmod behavior: Lmod uses any path as specified, so non-existing, empty or "unusual" ones are just used.
So the "fast" module use/unuse must use the non-cleaned path or behavior will differ

There's a reason we're filtering those here...

Remember which? Also the default is still to clean, so all of EB except for the new functions is unaffected

From Slack:

we do need a clean_existing_paths for curr_module_paths, but it should still filter out empty paths

Why? I don't even think we should clean out non-existing paths by default and filtering out empty paths would be different from what Lmod does, although I'm not sure about the semantic of that. And if we do filter those, then the fast and regular module.use would be different.

test/framework/easyconfig.py

boegel · 2021-04-07T12:27:17Z

easybuild/tools/modules.py


    def unuse(self, path):
        """Remove a module path"""
        # We can simply remove the path from MODULEPATH to avoid the costly module call


costly "module unuse" call

Maybe also clarify that modifying $MODULEPATH directly implies that Lmod won't reconsider loaded modules, but in the context of an EasyBuild session that should be OK...

Maybe we should also add an option to force running the actual module unuse command (same above for use), in case people are using the EasyBuild interface to the modules tool in Python scripts?

Done with default set to False to provide the fast implementation by default

easybuild/tools/modules.py

boegel · 2021-04-07T12:32:17Z

easybuild/tools/modules.py

            self.use(path, priority=priority)
+            modulepath = curr_module_paths(clean=False)
+            path_idx = modulepath.index(path)
+            if path_idx != 0:


If we notice the path isn't first, can't we try again with self.use(path, priority=priority, force_module_command=True)?
The warning should suffice to make it clear something fishy is going on, but we can still try to do the correct thing?

This wouldn't change anything: If priority is None, then the path will be at the front already due to the fast path, if priority is not None or any priorities are in use then the module command will be used already.

easybuild/tools/modules.py

akesandgren · 2021-05-26T18:04:07Z

@Flamefire Conflict resolution needed

Flamefire · 2021-05-27T07:02:27Z

Rebased and conflicts resolved. That was quite a lot... Please double-check the changes

easybuild/tools/modules.py

test/framework/modules.py

akesandgren · 2021-05-27T12:53:13Z

This now looks good to me, but since it's large i'll wait for @boegel to review before merging.

akesandgren

LGTM

akesandgren · 2021-10-07T09:14:57Z

test/framework/modules.py

+            os.environ['MODULEPATH'] = old_module_path  # Restore
+
+            # Forcing to use the module command reloads modules (Only Lmod does this)
+            old_module_path = os.environ['MODULEPATH']


This line is superfluous, old_module_path is already == os.environ['MODULEPATH']

Actually the "os.environ... Restore" line above is also useless since you're setting it again below.

You are right, but those are more or less different (sub-)testcases. I wanted to make them self-contained, so it would still work if we remove one of those. Maybe best done via a fixture or context-manager but yeah...

FWIW: #5015 introduces a context manager that can be used here

Lmod ignores if a path exists so we should be able to do the same

Introduce _set_module_path which correctly handles empty paths and empty lists with proper logging

Flamefire · 2025-10-08T15:46:30Z

There is one failing test where we should consider what we want:

self.prepend_module_path(aa)
self.prepend_module_path(bb, 10000)
self.prepend_module_path(cc)

If the caller of cc doesn't know anything about the former states he reasonably expects to be first but is not.

This currently happens in a test: load(module, mod_paths=[aa, (bb,10000), cc]) and checks for bb being first while with this patch cc is first.

boegel · 2025-10-22T14:23:21Z

There is one failing test where we should consider what we want:
self.prepend_module_path(aa)
self.prepend_module_path(bb, 10000)
self.prepend_module_path(cc)
If the caller of cc doesn't know anything about the former states he reasonably expects to be first but is not.

This currently happens in a test: load(module, mod_paths=[aa, (bb,10000), cc]) and checks for bb being first while with this patch cc is first.

If a path was added with priority, that was likely done with good reason, so we should respect that...

This recently popped up actually, see c8e723a (which is part of #4991)

Flamefire · 2025-10-22T16:02:02Z

This could be easily done by removing:

            # If no explicit priority is set, but priorities are already in use we need to use a high
            # priority to make sure the path (very likely) ends up at the front
            if priority is None and self._has_module_paths_with_priority():
                priority = self.HIGH_PRIORITY

I can do that.

…prepend_module_path

Flamefire · 2025-10-23T12:05:31Z

This is no longer useful as priorities need to be used when specified and so we might end up with prepend_module_path not prepending paths and we cannot fix that, see the above comments

I kept the relevant changes from this PR and opened a new one: #5030

@boegel This includes 2 important bugfixes I discovered with the added tests

Flamefire force-pushed the fasterPrependModulePath branch from 49093a6 to 9f82a1b Compare April 7, 2021 11:06

boegel added this to the release after 4.3.4 milestone Apr 7, 2021

boegel added change code cleanup performance labels Apr 7, 2021

boegel requested changes Apr 7, 2021

View reviewed changes

Flamefire mentioned this pull request Apr 14, 2021

Simplify use/unuse in Lmod #3634

Closed

boegel modified the milestones: 4.4.0, release after 4.4.0 May 26, 2021

Flamefire force-pushed the fasterPrependModulePath branch from 1d62274 to 59af049 Compare May 27, 2021 07:01

Flamefire force-pushed the fasterPrependModulePath branch from 59af049 to 2f85eaa Compare May 27, 2021 07:04

akesandgren requested changes May 27, 2021

View reviewed changes

akesandgren previously approved these changes May 27, 2021

View reviewed changes

akesandgren requested a review from boegel May 27, 2021 12:53

boegel modified the milestones: next release (4.4.1), release after 4.4.1 Jul 1, 2021

easybuilders deleted a comment from boegelbot Sep 1, 2021

boegel mentioned this pull request Sep 1, 2021

Don't use priority for module use #3631

Open

boegel modified the milestones: 4.4.2, release after 4.4.2 Sep 1, 2021

Flamefire dismissed akesandgren’s stale review via 735c24c September 21, 2021 07:29

Flamefire force-pushed the fasterPrependModulePath branch from 926efa6 to 735c24c Compare September 21, 2021 07:29

akesandgren requested changes Oct 7, 2021

View reviewed changes

boegel modified the milestones: 4.5.0, release after 4.5.0 Oct 13, 2021

boegel removed this from the 4.5.1 milestone Dec 7, 2021

boegel added this to the release after 4.9.0 milestone Dec 26, 2023

boegel modified the milestones: 4.9.1, 5.0 Apr 3, 2024

Flamefire added 10 commits April 16, 2024 10:11

Use mk_module_path

f16afdb

Add clean flag to curr_module_paths to not strip any paths

5bb2216

Lmod ignores if a path exists so we should be able to do the same

Unify handling of setting the MODULEPATH in Lmod

b3077ef

Introduce _set_module_path which correctly handles empty paths and empty lists with proper logging

Use dir not prefix for mkdtemp

bd1e791

Avoid use of priority in prepend_module_path

318576e

Factor out use of os.environ.get('__LMOD_Priority_MODULEPATH')

75802a2

Upgrade prepend failed warning to print_warning

18f20cb

Add force_module_command parameter

f4bd893

Fix normalization when checking path in curr_module_paths

097d3c7

Use os.pathsep consistently

a503b87

Flamefire force-pushed the fasterPrependModulePath branch from 670ea75 to a503b87 Compare April 16, 2024 08:11

boegel modified the milestones: 5.0.0, 5.x Mar 17, 2025

Merge branch 'develop' into fasterPrependModulePath

8f57794

Flamefire dismissed akesandgren’s stale review via 8f57794 March 20, 2025 13:37

Merge branch 'develop' into fasterPrependModulePath

de9a816

Flamefire added 2 commits October 23, 2025 12:50

Merge branch 'develop' into fasterPrependModulePath

e0cb9fc

Allow higher priority MODULEPATHs to be in front of paths added with …

6e11808

…prepend_module_path

Flamefire changed the title ~~Avoid using a priority in prepend_module_path (Lmod) to avoid costly module calls~~ Directly modify LMod $MODULEPATH to avoid costly module calls Oct 23, 2025

Flamefire closed this Oct 23, 2025

Flamefire changed the title ~~Directly modify LMod $MODULEPATH to avoid costly module calls~~ Avoid using a priority in prepend_module_path (Lmod) to avoid costly module calls Oct 23, 2025

Flamefire mentioned this pull request Oct 23, 2025

Fix handling of priorities and path normalisation in LMod $MODULEPATH and show warning on failed prepending #5030

Open

Flamefire deleted the fasterPrependModulePath branch October 23, 2025 12:03

Avoid using a priority in prepend_module_path (Lmod) to avoid costly module calls #3636

Avoid using a priority in prepend_module_path (Lmod) to avoid costly module calls #3636

Uh oh!

Conversation

Flamefire commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boegel commented Apr 7, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akesandgren commented May 26, 2021

Uh oh!

Flamefire commented May 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akesandgren commented May 27, 2021

Uh oh!

akesandgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire commented Oct 8, 2025

Uh oh!

boegel commented Oct 22, 2025

Uh oh!

Flamefire commented Oct 22, 2025

Uh oh!

Flamefire commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Flamefire commented Apr 7, 2021 •

edited

Loading