Skip to content

fix issue that parallel gets set to None with EB 5+ #1104

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix issue that parallel gets set to None with EB 5+ #1104

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66252c6
fix issue that parallel gets set to None with EB 5+
truib May 28, 2025
99d8f1b
different workaround for missing setting in EB5+
truib May 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions eb_hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ def post_ready_hook(self, *args, **kwargs):
parallel_param = 'max_parallel'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better with EasyBuild 5.x would be to use self.cfg.parallel instead of self.cfg['parallel']

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think the information gathering here is in the wrong place. You only need to do this if you plan to change the parallelism, so this whole block could be moved to when you need it, right before if new_parallel != parallel:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, operation_func requires it (right now), but at it should at least be within the if statement

# get current parallelism setting
parallel = self.cfg[parallel_param]
if parallel == None:
return # self.cfg doesn't contain 'parallel' or 'max_parallel'
Copy link
Member

@ocaisa ocaisa Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exits too early, it may be the case that this is None but you still want to limit the parallelism. What we really want is to call https://github.com/easybuilders/easybuild-framework/pull/3811/files#diff-8136c25c2706ba344aa622b3441a997a04037c1f5e6acadc006ae27f132554f4R1833 since that covers all cases.

That's an open framework PR though so I can be pragmatic. In general this query is triggered for every easyconfig, but in reality you only need to know it if you plan on changing it. The query should happen inside the if block. In that case you'll only see the warning when it affects what you are trying to build (right now you see it on every call to eb). I'm ok with reverting #1089 as long as the querying of the parallelism is moved into the if self.name in PARALLELISM_LIMITS: block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think getattr(self, 'parallel', self.cfg['parallel']) will work for both scenarios without the need for a version check.

if parallel == 1:
return # no need to limit if already using 1 core

Expand Down
Loading