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

Deprecate use of ec['parallel'] and fix updating the template value #3842

Closed
wants to merge 3 commits into from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Sep 20, 2021

ec['parallel'] currently doubles as an EC option and as the storage for the calculated parallelism set by the EasyBlock.
This makes it hard to reason about especially as maxparallel has pretty much the same effect. Also changes to ec['parallel'] done by e.g. easyblocks (or the set_parallel method) are not reflected by the template %(parallel)s

Solution: Introduce a property which on write updates the template and some magic to mirror the effect of the now deprecated ec['parallel']

Additional change I'd like to do: Treat parallel = False (legacy) as maxparallel =1 so cfg.parallel is always a number

Requires easybuilders/easybuild-easyconfigs#19374 to avoid deprecation warnings

See #3811 (comment) for the motivation

@boegel
Copy link
Member

boegel commented Sep 29, 2021

Makes sense in general, but haven't reviewed this in detail yet (I'm wondering whether we can avoid the parallelLegacy part).

@Flamefire Have you looked into the impact that this will have on both easyblocks (where self.cfg['parallel'] is used) and easyconfigs (where any that use parallel = X will basically be broken because of this)?

I have a feeling that the way this is done currently is too "hard" of a change, we should do it in a way that just results in a warning when self.cfg['parallel'] or parallel = X are being used, we shouldn't break anything...

@Flamefire
Copy link
Contributor Author

I have a feeling that the way this is done currently is too "hard" of a change, we should do it in a way that just results in a warning when self.cfg['parallel'] or parallel = X are being used, we shouldn't break anything...

This PR is exactly that: NOTHING is broken by that. I took exceptionally great care to make sure of that which requires the parallelLegacy part. Without this it is impossible to NOT break stuff, due to the possibility of custom EasyBlocks.
To answer your questions:

Have you looked into the impact that this will have on both easyblocks (where self.cfg['parallel'] is used)

This will continue to work as before as that value will be stored in parallelLegacy instead (transparently) and a deprecation warning will be shown which tells you to use self.cfg.parallel instead.

easyconfigs (where any that use parallel = X will basically be broken because of this)?

They will also work exactly as before with a deprecation warning that maxparallel should be used instead but otherwise there is no change as the transition var parallelLegacy is used: https://github.com/easybuilders/easybuild-framework/pull/3842/files#diff-00260ae7a519d5825760f53b067b29fb84a3e0d2649e6a27ace99abaca96d7d1R1819

# Storage for parallel property. Mark as unset initially
self._parallel = None
# Legacy value, remove with EasyBuild 5.0
self._config['parallelLegacy'] = [None, '', ('', )]
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this, since this opens the door to letting easyblocks use self.cfg['paralleLegacy']...

Can we avoid parking this in the self._config dictionary, and use self._parallel_legacy instead?

If not, we should rename 'paralleLegacy' to something more obscure that better signals that it's an internal thing (like '_parallel_legacy')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we avoid parking this in the self._config dictionary, and use self._parallel_legacy instead?

Not really as it needs to be in the EC instance not the EB instance. Played around with it a bit and this was the best solution to keep the change as transparent and backwards compatible as possible.
But yes we can rename it although I chose paralleLegacy to be already obscure enough to not be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the requested change after the latest rebase using self._config['_parallelLegacy']

@Flamefire
Copy link
Contributor Author

@boegel What was the issue here? Anything remaining?

@Flamefire Flamefire requested a review from boegel August 23, 2022 11:46
@easybuilders easybuilders deleted a comment from boegelbot Dec 20, 2023
@easybuilders easybuilders deleted a comment from boegelbot Dec 20, 2023
@boegel boegel modified the milestones: next release (4.9.0?), 5.0 Dec 20, 2023
@boegel boegel added EasyBuild-5.0 EasyBuild 5.0 and removed EasyBuild-5.0 EasyBuild 5.0 labels Dec 20, 2023
@boegel boegel modified the milestones: 4.9.1, release after 4.9.1 Apr 3, 2024
@Flamefire Flamefire force-pushed the deprecate_parallel branch from b0e05d3 to bf6b637 Compare April 16, 2024 08:03
@Flamefire
Copy link
Contributor Author

@boegel *ping

ec['parallel'] currently doubles as an EC option and as the storage for
the calculated parallelism set by the EasyBlock.
This makes it hard to reason about especially as maxparallel has pretty
much the same effect. Also changes to ec['parallel'] done by e.g.
easyblocks (or the set_parallel method) are not reflected by the
template `%(parallel)s`

Solution: Introduce a property which on write updates the template and
some magic to mirror the effect of the now deprecated ec['parallel']
Migrate from `self.cfg['parallel']` to `self.cfg.parallel`
@Flamefire Flamefire force-pushed the deprecate_parallel branch from 0f19be2 to b7c7706 Compare July 17, 2024 14:54
@boegel boegel modified the milestones: 4.9.3, release after 4.9.3 Aug 28, 2024
@boegel
Copy link
Member

boegel commented Nov 6, 2024

@Flamefire This can be closed in favor of #4580, right?

@Flamefire
Copy link
Contributor Author

If there won't be another 4.x then yes.

@boegel boegel closed this Jan 29, 2025
@Flamefire Flamefire deleted the deprecate_parallel branch February 2, 2025 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants