Avoid failure in Python package installation and sanity check when $PIP_REQUIRE_VIRTUALENV is set#3460
Conversation
8b6f66a to
03c35b4
Compare
| # 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) |
There was a problem hiding this comment.
@Flamefire Shouldn't we introduce a function that can be called from both pythonbundle and pythonpackage?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
66b3699 to
7c6bfc9
Compare
$PIP_REQUIRE_VIRTUALENV is set
|
@boegelbot please test @ jsc-zen3-a100 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 2401832507 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
$PIP_REQUIRE_VIRTUALENV is set$PIP_REQUIRE_VIRTUALENV is set
|
@boegel ping on that. Just ran into this failure while testing EB 5 ECs. |
|
Rebased for 5.0x |
|
Can this be included in 5.0.1? I had to patch our 5.0.0 installation again after the update. And test reports looked fine too so I don't expect any downsides. |
I'll see what I can do to get this merged ASAP |
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 5 out of 5 (5 easyconfigs in total) |
Consolidates the multiple individual usages into a single function.
|
Rebased without changes to resolve merge conflict |
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