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

Stop importing pip, use subprocesses instead. Will close #710 #797

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

dwpaley
Copy link
Contributor

@dwpaley dwpaley commented Oct 4, 2022

As discussed in: pypa/setuptools#3297 it seems that importing pip is just not supported. Our use case was not really compelling, we were (1) checking the version and (2) using a pip._internal.main api which is a thin (and to be deprecated) wrapper over pip._internal.cli.main.main There's a clear discussion here: https://pip.pypa.io/en/stable/user_guide/#using-pip-from-your-program of why using pip via subprocess is preferred.

This is a draft because there's a second place in libtbx/auto_build/install_base_packages.py where pip._internal.main is used in a similar way.

@dwpaley dwpaley requested a review from bkpoon October 4, 2022 19:52
@dwpaley dwpaley marked this pull request as draft October 4, 2022 19:52
@dwpaley dwpaley marked this pull request as ready for review October 4, 2022 20:48
@dwpaley
Copy link
Contributor Author

dwpaley commented Oct 4, 2022

In principle this is done, but I don't know how to exercise the build_python_module_pip method in install_base_packages.py...

Copy link
Member

@bkpoon bkpoon left a comment

Choose a reason for hiding this comment

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

The install_base_packages.py code is only really for building Python 2.7 dependencies and I doubt that it will work now since we do not specify the versions of every dependency. Probably, one dependency will be installed via pip which pulls in an incompatible dependency that we did not version.

But you can try to build a minimal set of Python 2.7 dependencies with

python bootstrap.py base --builder=cctbxlite

libtbx/auto_build/conda_envs/cctbx.devenv.yml Outdated Show resolved Hide resolved
remove duplicate `packaging` dependency
@dwpaley dwpaley requested a review from JBlaschke October 5, 2022 18:28
@dwpaley
Copy link
Contributor Author

dwpaley commented Oct 5, 2022

@JBlaschke From Billy:

maybe you will want to test the XFEL/DIALS builds at NERSC since the pkg_utils.py code is used for setting up all those entry points. The XFEL CI might be enough, but I don't know if Johannes' installation scripts do anything extra.

@JBlaschke
Copy link
Contributor

Hi Folks, I am still waiting to test this on Perlmutter (longer than expected maintenance), but I can say that it works on Cori.

Copy link
Contributor

@JBlaschke JBlaschke left a comment

Choose a reason for hiding this comment

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

This looks like a run-of-the-mill replacement of the pip module with a sub-process call. The only concern RE HPC is if you tried that with MPI then you might get into trouble because MPI (and libfabric) are not considered fork safe, while subprocess will call fork. I don't think that this will be an issue for an installer however -- so I think it's safe to proceed.

Another thing to consider is that this might be called within a workflow management tool. In that case we need to make sure that the workflow tool exposes the correct environment to all children. Most workflow tools use fork or execve so we should be safe there, and I would counsel vigilance in case we see the "wrong" pip being picked up.

@JBlaschke
Copy link
Contributor

@dwpaley for cleanliness have you or @bkpoon considered: https://github.com/amoffat/sh

@JBlaschke
Copy link
Contributor

@bkpoon I have a question about the most elegant way to tell bootstrap.py to use a specific branch: Right now I edit boostrap.py manually to add the -b <branch name> to the module. For someone who uses boostrap.py deploy and build in one single command, is there an argument that I can use to do straight to a specific module's branch?

@dwpaley dwpaley merged commit 8a0b852 into master Oct 11, 2022
dwpaley added a commit that referenced this pull request Oct 11, 2022
@dwpaley dwpaley deleted the stop_importing_pip branch October 20, 2022 18:05
Trzs pushed a commit that referenced this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants