-
Notifications
You must be signed in to change notification settings - Fork 219
Deprecate use of parallel easyconfig parameter and fix updating the template value
#4580
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
Changes from 11 commits
862e797
113820a
d2abfbb
162621f
da9d5e5
c5be463
ba66029
ed8d0ae
3ad6aa5
48ea737
d79ffe0
5bd793f
192a329
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -120,7 +120,17 @@ def handle_deprecated_or_replaced_easyconfig_parameters(ec_method): | |||||
| def new_ec_method(self, key, *args, **kwargs): | ||||||
| """Check whether any replace easyconfig parameters are still used""" | ||||||
| # map deprecated parameters to their replacements, issue deprecation warning(/error) | ||||||
| if key in ALTERNATIVE_EASYCONFIG_PARAMETERS: | ||||||
| if key == 'parallel': | ||||||
| _log.deprecated("Easyconfig parameter 'parallel' is deprecated, " | ||||||
| "use 'max_parallel' or the parallel property instead.", '5.1') | ||||||
|
||||||
| "use 'max_parallel' or the parallel property instead.", '5.1') | |
| "use 'max_parallel' or the parallel property instead.", '6.0') |
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.
My idea was to remove this with EB 5 but it got delayed too much to have a deprecation period.
I still wanted to use EB 5 to introduce this breaking change especially as we already have many other breaking changes in EB 5 not preceded by deprecation periods.
That it is not a breaking change in 5.0 is to allow a short period for updating even though it is trivial update on the users side and our shipped easyblocks and easyconfigs are already updated.
Hence the (very short) deprecation period to just to make the transition smoother. Delaying it to EB 6 means we'll keep the clunky and not fully intuitive workaround for years to come.
I wouldn't do that.
So why not have it as a "soft breaking" change in EB 5 to make our life easier?
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.
We only make breaking changes in major versions, I don't want to step away from that.
The workaround is there now, keeping it around for a while is not that bad, there's plenty of other warts in the code we've lived with for a long time.
We've also discussed that "the years to come" will hopefully not be as long as it was between EasyBuild 4.0 and 5.0.
boegel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
boegel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
boegel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
| # In particular writes to ec['parallel'] do NOT update the %(parallel)s template. | |
| # In particular writes to self.cfg['parallel'] do NOT update the %(parallel)s template. |
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 wanted to be more general. self.cfg is only defined in easyblocks and not in e.g. hooks or whatever else might get an easyconfig 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.
90% of the time in hooks it's also self.cfg, so using the most common case made more sense to me
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.
no need to mention here when this will be removed, see
handle_deprecated_or_replaced_easyconfig_parametersThere 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 wanted to keep a hint/TODO to remove the workaround involving the use of
_parallelLegacycompletely, in this case the whole block of 6 lines. Not sure if this was clear enough fromhandle_deprecated_or_replaced_easyconfig_parametersbut I guess your improved comment might be.