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

Permit empty environment variables as unset in setup.py #4185

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

loadams
Copy link
Contributor

@loadams loadams commented Aug 21, 2023

This change fixes a bug that causes setup.py to fail when any of its configuration environment variables are the empty string.

For example, running the following:

export DS_BUILD_OPS=""
pip install deepspeed

Currently fails with the following message:

BUILD_OP_DEFAULT = int(os.environ.get('DS_BUILD_OPS', BUILD_OP_PLATFORM))
ValueError: invalid literal for int() with base 10: ''

Because os.environ.get does not use its fallback argument when an environment variable exists, but is empty.
Similarly, key in os.environ returns True for empty (but still set) keys.

Shells usually do not distinguish between unset variables and empty variables, so setting variables to the empty string is usually seen as another way to unset them. This specifically arose as an issue for me while I was writing a Dockerfile, since those have no way to unset ENV and ARG directives—only to set them to the empty string (or to figure out and hardcode what the default would have resolved to for each ARG). I figure fixing this here is more helpful than writing weird workarounds.

This changes setup.py to treat unset and empty environment variables identically. This behaviour could potentially be changed in other files, as well, but that is not included here.

Co-authored-by: Eta0 Eta0@users.noreply.github.com

@loadams loadams requested review from mrwyattii and jeffra August 21, 2023 17:26
@loadams
Copy link
Contributor Author

loadams commented Aug 21, 2023

This is a follow up from #3763

@loadams loadams changed the title Treat empty environment variables as unset in setup.py Permit empty environment variables as unset in setup.py Aug 21, 2023
@loadams loadams enabled auto-merge August 21, 2023 18:36
@loadams loadams added this pull request to the merge queue Aug 21, 2023
Merged via the queue into master with commit 8fb111c Aug 22, 2023
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.

2 participants