fix issue that parallel gets set to None with EB 5+ #1104
Conversation
trz42
added
bug
|
Instance
|
|
Just need to build this once (for a single architecture) ... |
Updates by the bot instance
|
|
New job on instance
|
eb_hooks.py
Outdated
| @@ -133,7 +133,8 @@ def post_ready_hook(self, *args, **kwargs): | |||
| # Check whether we have EasyBuild 4 or 5 | |||
| parallel_param = 'parallel' | |||
| if EASYBUILD_VERSION >= '5': | |||
| parallel_param = 'max_parallel' | |||
| # parallel_param = 'max_parallel' # EasyBlock.set_parallel sets 'parallel' | |||
| parallel_param = 'parallel' | |||
There was a problem hiding this comment.
This just reverts the previous change and reintroduces the warning about using a deprecated parameter
There was a problem hiding this comment.
I think we need a better solution as the warning appears for anyone using EESSI-extend
There was a problem hiding this comment.
Yeah, but self.cfg['max_parallel'] doesn't exist plus --max-parallel seems not fully working in EB 5+.
Could we disable that specific warning?
There was a problem hiding this comment.
Is the warning really appearing for anyone or only when we adjust the parallelism in the post_ready_hook?
There was a problem hiding this comment.
Why don't we just use the parallel property (I'm guessing that is self.parallel) then in the EB5 scenario?
There was a problem hiding this comment.
Is the warning really appearing for anyone or only when we adjust the parallelism in the post_ready_hook?
I think yes, I fixed it because it bothered me to see it all the time while preparing the CI tutorial
There was a problem hiding this comment.
Why don't we just use the parallel property (I'm guessing that is
self.parallel) then in the EB5 scenario?
Hmm, does that exist?
Trying it for TensorFlow I get
File "/local/scratch/roeblitz1/software-layer/eb_hooks.py", line 141, in post_ready_hook
print_msg("self.parallel = '%s'", self.parallel)
^^^^^^^^^^^^^
AttributeError: 'PythonBundle' object has no attribute 'parallel'
|
Suggesting a different fix/workaround ... |
Updates by the bot instance
|
|
New job on instance
|
| @@ -136,6 +136,8 @@ def post_ready_hook(self, *args, **kwargs): | |||
| parallel_param = 'max_parallel' | |||
There was a problem hiding this comment.
A better with EasyBuild 5.x would be to use self.cfg.parallel instead of self.cfg['parallel']
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Ah sorry, operation_func requires it (right now), but at it should at least be within the if statement
| @@ -136,6 +136,8 @@ def post_ready_hook(self, *args, **kwargs): | |||
| parallel_param = 'max_parallel' | |||
| # get current parallelism setting | |||
| parallel = self.cfg[parallel_param] | |||
| if parallel == None: | |||
| return # self.cfg doesn't contain 'parallel' or 'max_parallel' | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think getattr(self, 'parallel', self.cfg['parallel']) will work for both scenarios without the need for a version check.
|
This is now being fixed via EESSI/software-layer-scripts#17, so let's close? |
|
Closing as suggested because issue is handled via EESSI/software-layer-scripts#17 |
Fixes an issue caused by the post_ready_hook trying to obtain the parallelism via
which is not set by
EasyBlock.set_parallel. The latter always (for EB 4 and EB 5) setsor