-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[runtime env] Make env_vars take effect when pip install packages #22730
Conversation
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.
Can we also make this apply to conda
while we're at it?
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.
Thanks for this feature, and nice test! In my opinion it's okay to leave the conda case for the future. We haven't had anyone ask about environment variables for conda, but several users have asked about it for pip
.
await check_output_cmd(pip_install_cmd, logger=logger, cwd=cwd, env={}) | ||
pip_env = os.environ.copy() | ||
pip_env.update(env_vars) | ||
await check_output_cmd(pip_install_cmd, logger=logger, cwd=cwd, env=pip_env) |
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.
In the common case, env_vars
is empty, so here we would be changing env={}
to env=os.environ.copy()
. These two should behave exactly the same with check_output_cmd
, right? Just want to double check.
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.
the argument env means overwrite not update, for example, you can run this command:
python test_subprocess.py
test_subprocess.py:
import subprocess
print(subprocess.check_output(["env"], env={}).decode("utf-8"))
It will print nothing, env=None
and env=os.environ.copy()
has same behave, you can verify it by the above command.
maybe I should open a new PR to make |
f.writelines(python_code) | ||
|
||
gz_filename = os.path.join(tmpdir, package_name + ".tar.gz") | ||
subprocess.check_call(["tar", "-zcvf", gz_filename, package_name]) |
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.
Good test case!
|
…y-project#22730) Previously, for the stability of pip installation, we set env to empty, but when pip installs some gzip package, maybe need env_vars. like this issue: ray-project#22610
Why are these changes needed?
Previously, for the stability of pip installation, we set env to empty, but when pip installs some gzip package, maybe need env_vars. like this issue: #22610
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.