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

Enable module-depends-on by default #4500

Merged
merged 14 commits into from
Oct 7, 2024
Merged

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Apr 4, 2024

I suspect I'll need to look into the tests that probably broke from this change.

I'm a bit unsure what to make of the cli arguments, given the store_true behavior is a bit strange when the default is True. But I do see other flags do this, like cleanup-tmpdir so i guess we want to keep this for module-depends-on as well.

fixes #4397

@Micket Micket added change EasyBuild-5.0 EasyBuild 5.0 labels Apr 4, 2024
@Micket Micket added this to the 5.0 milestone Apr 4, 2024
@Micket Micket force-pushed the moduledependson branch from d93ba00 to f27a7fc Compare April 5, 2024 23:47
@Micket
Copy link
Contributor Author

Micket commented Apr 6, 2024

well, apparently, can't enable it by default since because environment modules. Ugh. Tests were not fun either, and going to be even less fun to consider the two different modes here for all the checks.

I thought about making it "none" and enable it by default if Lmod is used, but, there is also the config file, what should one write. I'm considering just giving up on this.

@boegel boegel added the EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 label Jun 5, 2024
@boegel boegel force-pushed the moduledependson branch from 951a0b4 to 794c068 Compare June 5, 2024 18:15
@boegel
Copy link
Member

boegel commented Jun 5, 2024

One problem here is that the Tcl implementation of environment modules only supports depends-on from v5.1.0 onwards.

#4425 proposes to bump the version requirement to v4.3.0+, which is still a long way off.

So either we come up with a sensible approach to having --module-depends-on enabled by default while still using load statements when using Tmod, or we give up on enabling this by default (for now)...

@xdelaruelle
Copy link
Contributor

xdelaruelle commented Aug 18, 2024

Do you foresee any valid use case where depends_on should not be used when module tool is Lmod? If no, why not just simply remove this option and always use depends_on when module tool is Lmod?

EnvironmentModules will be fine on its side with module load commands to handle the dependencies between modules.

@boegel
Copy link
Member

boegel commented Aug 26, 2024

With Tmod, we could only use depends_on when it's supported by the Tmod version being used

@xdelaruelle
Copy link
Contributor

With Tmod, we could only use depends_on when it's supported by the Tmod version being used

Is there a need to make the module-depends-on option also apply when selected ModuleTool is EnvironmentModules? Tmod is able to correctly handle dependencies with module load. On Tmod, depends-on is just some kind of alias over module load, but as you say this alias appeared on version 5.1.

If this option only applies when selected ModuleTool is Lmod (to build either Tcl or Lua modulefiles), you will be able to enable this option by default.

Relaxing the constrain to use same command whatever the kind of ModuleTool used, may help you here.

@boegel
Copy link
Member

boegel commented Aug 27, 2024

I'm fine with @xdelaruelle's suggestion here, to only let the --module-depends-on configuration option being enabled trigger the use of depends_on when Lmod is used as modules tool...

@Micket Thoughts on that approach?

@Micket
Copy link
Contributor Author

Micket commented Aug 27, 2024

Well https://www.youtube.com/watch?v=CxK_nA2iVXw

I think it's mostly just how ugly the tests are gonna be when it needs to consider two different options. If I'm forced to pick, I'd say just let it complain on older Tmod and we just tread this option as stated, using depends-on for both.

@xdelaruelle
Copy link
Contributor

I'd say just let it complain on older Tmod and we just tread this option as stated, using depends-on for both.

If you proceed this way and as this option will be enabled by default, you will break the EasyBuild installation of all people using Tmod > 4.3 < 5.1.

Having a configuration option that only triggers change on one side of the module ecosystem already exists with module-extensions (that only triggers change on Lua modulefiles).

From what I have seen of the tests, they are already handling a good level of complexity. So having module-depends-on only affect modulefiles generated for Lmod should not increase this complexity too much.

If this is a real concern though, I would just suggest to drop the module-depends-on option: I do not see a need to process module dependencies with other things than depends_on on Lmod. I do not practice Lmod as you do, so you may know of some corner cases where depends_on is bad. But if you do not foresee any issue with depends_on, it will be much more simpler for you to just drop this option and its complexity and just using depends_on if ModuleTool is Lmod and module load if ModuleTool is EnvironmentModules.

@Micket Micket force-pushed the moduledependson branch 3 times, most recently from 3fadae0 to 4842b96 Compare September 18, 2024 13:08
@Micket Micket marked this pull request as ready for review September 18, 2024 13:13
@Micket Micket force-pushed the moduledependson branch 6 times, most recently from 582dc39 to c9c5395 Compare September 18, 2024 17:25
fix tests that got broken by enabling use of depends_on (instead of load) statement in generated environment modules by default
@Micket Micket force-pushed the moduledependson branch 2 times, most recently from c65b741 to c015187 Compare September 19, 2024 08:39
@Micket
Copy link
Contributor Author

Micket commented Sep 19, 2024

Sigh, this is getting unnecessarily complicated

I've been asked to preserve much of the old behavior, but given that:

  1. one can set depends-on via command line and global config
  2. one can set the optional keyword argument to the module_generator function
  3. one can also set the parameter inside easyconfig files, which then would override the default keyword argument above.
  4. also change behavior whether the module tool supports it, and I've been asked to preserve the behavior that it should error or, if it's set by option 2 and 3, but not 1.

These constraints now puts my in an impossible situation as to what the logic should be (not difficult to figure out; literally impossible constraints). And whatever the logic ends up being, the meaning of all these identically named depends_on parameters have different semantics.

I'm proposing we instead get rid of the easyconfig and keyword argument. Only using the build option. That's it.
I can't imagine any scenario where you wish to control the module dependency behavior per easyconfig. Plenty of things you can't control regarding how the module is generated, and I see no particular need for this one. We have never used it in any upstream easyconfigs.

@boegel boegel changed the title Enable module-depends-on by default Enable module-depends-on by default Oct 7, 2024
@boegel boegel merged commit 96ff24b into easybuilders:5.0.x Oct 7, 2024
36 checks passed
@Micket Micket deleted the moduledependson branch October 7, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 EasyBuild-5.0 EasyBuild 5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants