enable download_dep_fail, use_pip, sanity_pip_check by default in PythonPackage easyblock#3022
Conversation
…honPackage easyblock
| Determine install command to use. | ||
| """ | ||
| if self.cfg.get('use_pip', False) or self.cfg.get('use_pip_editable', False): | ||
| if self.cfg.get('use_pip', True) or self.cfg.get('use_pip_editable', False): |
There was a problem hiding this comment.
Do we still need this duplication of the default params in EB 5.x or could we just switch to self.cfg['use_pip'] and fix EasyBlocks not "inheriting" the options this block requires?
There was a problem hiding this comment.
I think it's wise to keep using self.cfg.get, to avoid hard crashes in easyblock to derive from PythonPackage but do not inherit the custom easyconfig parameters from PythonPackage.
We can make sure the easyblocks we have in the central repository do so, but we don't control all easyblocks out there, and it's a small effort to avoid fallout...
The self.cfg.get is a pattern that we should use in all generic easyblocks (and even some custom ones) since they may be used as a base for other easyblocks.
There was a problem hiding this comment.
I understand that motivation for EB 4.x but I wanted to suggest that such a breakage could be OK for EB 5.x. Not inheriting the parameters has several other implications such as not being able to list available parameters correctly. This introduces some kind of "hidden" parameters and puts burden on upstream EB to be careful to always use the self.cfg.get pattern and deal with potential different default values. Technically each instance of those self.cfg.get calls would need a check against the extra_parameters section that the default matches and that possibly even across easyblocks (some descendant of e.g. PythonPackage also using self.cfg.get('use_pip' would possibly need to be updated too)
There was a problem hiding this comment.
@Flamefire It's a good point, and I'm happy to follow up on it, but we should do that consistently then, so let's open an issue on this.
The window of opportunity w.r.t. (potentially) breaking changes to include in EasyBuild v5.0 is slowly closing though...
There was a problem hiding this comment.
Yes, I agree that it is a good point. Let's follow up on this topic in a separate issue or PR. I will merge this PR as is so we can continue testing the 5.0.x branch for EasyBuild v5.0.
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) |
|
List of easyconfig files currently in
|
…pip, since PythonPackage now enables use_pip by default
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 29 out of 29 (29 easyconfigs in total) |
|
Test report by @SebastianAchilles Overview of tested easyconfigs (in order)
Build succeeded for 7 out of 7 (7 easyconfigs in total) |
|
Going in, thanks @boegel! |
Probably worth doing a couple of test builds for the easyblocks that are touched, to avoid surprises.
Easyconfigs which are currently enabling these easyconfig parameters should be cleaned up.
Non-archived easyconifgs which are not enabling these easyconfig parameters yet should be updated to set them to
False(unless usingpipand running "pip check" works fine for them).fixes #2127