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.

Sign up for GitHub

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

Jump to bottom

Avoid failure in Python package installation and sanity check when $PIP_REQUIRE_VIRTUALENV is set #3460

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Avoid failure in Python package installation and sanity check when $PIP_REQUIRE_VIRTUALENV is set #3460

wants to merge 2 commits into from

Conversation

Flamefire
Copy link
Contributor

We already have it for the installation if set by users but if a module sets it (e.g Python) the sanity check will fail

@boegel boegel added the bug fix label Sep 25, 2024
@boegel boegel added this to the 4.x milestone Sep 25, 2024
boegel
boegel requested changes Sep 25, 2024
View reviewed changes
easybuild/easyblocks/generic/pythonbundle.py Outdated
@@ -181,6 +181,8 @@ def load_module(self, *args, **kwargs):
# Required here to ensure that it is defined for sanity check commands of the bundle
# because the environment is reset to the initial environment right before loading the module
env.setvar('PYTHONNOUSERSITE', '1', verbose=False)
# Set (again) in case the module changes it
env.setvar('PIP_REQUIRE_VIRTUALENV', 'false', verbose=False)
Copy link
Member

Choose a reason for hiding this comment

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

@Flamefire Shouldn't we introduce a function that can be called from both pythonbundle and pythonpackage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean one that sets PYTHONNOUSERSITE, PIP_REQUIRE_VIRTUALENV and PIP_DISABLE_PIP_VERSION_CHECK?

Given that we already need it in quite some places it makes sense, yes. Maybe named setup_pip_env with a verbose param? I'd make that default to False and only set it to True in PythonPackage.__init__ to reduce log spam. Even there it gets likely duplicated a lot for bundles but completely omitting it feels wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, implemented it in the Python EasyBlock file to avoid cyclic includes of that and PythonPackage. Those variables are also required/useful for TensorFlow & friends, hence I made them a constant.

To avoid the log spam the actual set is only executed if any of the variables need changing.

@Flamefire
Factor out method for setting variables for Python package installations
7c6bfc9 
Consolidates the multiple individual usages into a single function.
@boegel boegel changed the title Avoid failure in Python sanity check when PIP_REQUIRE_VIRTUALENV is set Avoid failure in Python sanity check when $PIP_REQUIRE_VIRTUALENV is set Oct 9, 2024
@boegel
Copy link
Member

boegel commented Oct 9, 2024

@boegelbot please test @ jsc-zen3-a100
EB_ARGS="tensorflow-compression-2.11.0-foss-2022a-CUDA-11.7.0.eb TensorFlow-2.15.1-foss-2023a.eb --installpath /tmp/$USER/pr3460"

@boegelbot
Copy link

@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de

PR test command 'if [[ develop != 'develop' ]]; then EB_BRANCH=develop ./easybuild_develop.sh 2> /dev/null 1>&2; EB_PREFIX=/home/boegelbot/easybuild/develop source init_env_easybuild_develop.sh; fi; EB_PR=3460 EB_ARGS="tensorflow-compression-2.11.0-foss-2022a-CUDA-11.7.0.eb TensorFlow-2.15.1-foss-2023a.eb --installpath /tmp/$USER/pr3460" EB_REPO=easybuild-easyblocks EB_BRANCH=develop /opt/software/slurm/bin/sbatch --job-name test_PR_3460 --ntasks=8 ~/boegelbot/eb_from_pr_upload_jsc-zen3.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 5038

Test results coming soon (I hope)...

- notification for comment with ID 2401832507 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link

Test report by @boegelbot

Overview of tested easyconfigs (in order)

  • SUCCESS tensorflow-compression-2.11.0-foss-2022a-CUDA-11.7.0.eb
  • SUCCESS TensorFlow-2.15.1-foss-2023a.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
jsczen3c1.int.jsc-zen3.fz-juelich.de - Linux Rocky Linux 9.4, x86_64, AMD EPYC-Milan Processor (zen3), Python 3.9.18
See https://gist.github.com/boegelbot/4b82c97538296dae8d67d812cc0e9f72 for a full test report.

@Flamefire Flamefire changed the title Avoid failure in Python sanity check when $PIP_REQUIRE_VIRTUALENV is set Avoid failure in Python package installation and sanity check when $PIP_REQUIRE_VIRTUALENV is set Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.
3 participants
@Flamefire @boegel @boegelbot