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

Enable pip check by default #1865

Open
Flamefire opened this issue Dec 1, 2019 · 5 comments
Open

Enable pip check by default #1865

Flamefire opened this issue Dec 1, 2019 · 5 comments
Milestone

Comments

@Flamefire
Copy link
Contributor

Support for running pip check at the end of the installation of a Python Package was recently added after a serious issue was detected in TensorFlow where mismatching dependencies led to hard-to-detect crashs.

To protect from further problems like that costing lot's of hours and hence money I'd suggest to enable the pip-check by default (not disable as for now)

Proposed solution:

Short term make the enable option a mandatory check in the CI for new or updated ECs, similar to e.g. https over http checks. This way all new or updated ECs already work with pip check (or need to be fixed)

On a decent system (build power and storage) grep for all Python packages, and reinstall them as a baseline to verify they still work. Then switch the default to enable pip check, reinstall the Python packages and fix detected issues for newish ECs. Older ones (and/or ones that don't seem worth maintaining) can be left broken until someone complains, have pip check manually disabled or be removed.

Ccing @boegel

@boegel
Copy link
Member

boegel commented Dec 10, 2019

Requiring sanity_pip_check = True in new/updated easyconfigs in PRs certainly makes perfect sense, and that's a pretty easy change to make in test/easyconfigs/easyconfigs.py.

Switching to having sanity_pip_check enabled by default is less easy though, even if we go through the (time-consuming) effort of checking all current easyconfig files for Python packages.

First of all, the latter is not as straightforward as just checking easyconfigs that use PythonPackage or PythonBundle, since some custom easyblocks derive from those (TensorFlow is an example there).

Second, we do not control all easyconfig files out there, just the ones in the central repository that are shipped with EasyBuild.
If enabling pip check by default makes installations that (seemingly) worked fine before fail with an update of EasyBuild, that's a regression (which we try hard to avoid). You could argue the installation is broken anyway if pip check fails, but that's not entirely correct, since the software may only actually need the missing dependency for some obscure feature...

Finally, we're quite new to pip check ourselves, and we may run into surprises with it (like #1877 for example...).

So, I'm all for required a gradual switch to enabling pip check, but it should done with care (so switching to having it enabled by default should only be done with a new major version of EasyBuild).

@Flamefire
Copy link
Contributor Author

Requiring sanity_pip_check = True in new/updated easyconfigs in PRs certainly makes perfect sense, and that's a pretty easy change to make in test/easyconfigs/easyconfigs.py.

Great! Could you do that then? I'm not sure how to actually implement this given you'd need to check it in multiple places (exts, global, default opts)

So, I'm all for required a gradual switch to enabling pip check, but it should done with care (so switching to having it enabled by default should only be done with a new major version of EasyBuild).

Well yes. How about running it always, but downgrade to a warning which is always shown when sanity_pip_check is disabled? So users installing something see a nice warning coming up and can fix that or open issues? This allows to handle that "not as straightforward" process :)

@boegel
Copy link
Member

boegel commented Dec 15, 2019

Requiring sanity_pip_check = True in new/updated easyconfigs in PRs certainly makes perfect sense, and that's a pretty easy change to make in test/easyconfigs/easyconfigs.py.

Great! Could you do that then? I'm not sure how to actually implement this given you'd need to check it in multiple places (exts, global, default opts)

done, see easybuilders/easybuild-easyconfigs#9516

So, I'm all for required a gradual switch to enabling pip check, but it should done with care (so switching to having it enabled by default should only be done with a new major version of EasyBuild).

Well yes. How about running it always, but downgrade to a warning which is always shown when sanity_pip_check is disabled? So users installing something see a nice warning coming up and can fix that or open issues? This allows to handle that "not as straightforward" process :)

That makes sense, I see no harm in always running pip check (unless it's hard disabled?) and producing a big fat warning if it failed, especially since the output is pretty concise & clear.
It should be fairly easy to change that in the PythonPackage easyblock...

@Flamefire
Copy link
Contributor Author

What's the status of this? Could this be made the default soon or added to the to-be-changed defaults for EB 5?

@boegel boegel modified the milestones: 4.4.0, 5.0 May 26, 2021
@boegel
Copy link
Member

boegel commented May 26, 2021

This shouldn't be done before EasyBuild 5.0, I think... So I tagged it with the 5.0 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants