-
Notifications
You must be signed in to change notification settings - Fork 283
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
enable download_dep_fail
, use_pip
, sanity_pip_check
by default in PythonPackage
easyblock
#3022
enable download_dep_fail
, use_pip
, sanity_pip_check
by default in PythonPackage
easyblock
#3022
Conversation
…honPackage easyblock
@@ -417,7 +417,7 @@ def determine_install_command(self): | |||
""" | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
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.
lgtm
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 usingpip
and running "pip check
" works fine for them).fixes #2127