DeepSpeed icon indicating copy to clipboard operation
DeepSpeed copied to clipboard

Treat empty environment variables as unset in `setup.py`

Open Eta0 opened this issue 2 years ago • 1 comments

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.

Eta0 avatar Jun 17 '23 00:06 Eta0

@Eta0 - if you want to merge this you'll need to reply to the Microsoft contributor agreement in the comment above

loadams avatar Jun 28 '23 15:06 loadams

@loadams Got it. Currently waiting on approval from my company to agree to it.

Eta0 avatar Jun 28 '23 17:06 Eta0

@Eta0 any update? It would be great to get this merged

mrwyattii avatar Jul 11 '23 17:07 mrwyattii

Thank you all for your patience! I'm still waiting on legal approval, so I'll check in with them again.


Update: it should be wrapped up some time this week 🎇 then I can sign and we can get this merged. Yay!

Eta0 avatar Jul 11 '23 19:07 Eta0

Hi @Eta0 - no rush, but figured I'd follow up since its been a week!

loadams avatar Jul 24 '23 16:07 loadams

Hi @Eta0 - following up on this again, curious if you were able to get legal approval.

loadams avatar Aug 09 '23 15:08 loadams

@Eta0 - Since we'd like to incorporate these changes, we've pulled them into a PR here: #4185

loadams avatar Aug 21 '23 17:08 loadams