Conditionally quote env vars

[saurabhkoshatwar]

Resolves #6997

This PR conditionally quotes environment variable values—only wrapping those containing special characters (like parentheses) that could trigger bash errors. Safe values remain unquoted.

[qiuosier]

This fix avoided some errors. However, there is still error with variable containing double quotes.
For example, when the var is a valid JSON string,

var = "{\"a\": \"b\"}"

after quoting, it will become

"{"a": "b"}"

then, in bash

export var="{"a": "b"}"

will export the var as

{a: b}

which is no longer a valid JSON string.

Shall we use shlex.quote() instead?

[saurabhkoshatwar]

@loadams cpu-torch-latest ci stuck, please rerun

[tjruwase]

@saurabhkoshatwar, thanks for the PR. Is it possible to add a unit test here?
https://github.com/deepspeedai/DeepSpeed/blob/master/tests/unit/launcher/test_user_args.py

[saurabhkoshatwar]

cc @loadams @tjruwase

[loadams]

cc @loadams @tjruwase

Thanks @saurabhkoshatwar, will review. Curious on your thoughts on @qiuosier's comment?

Also can you take a look at the formatting errors?

[loadams]

This fix avoided some errors. However, there is still error with variable containing double quotes. For example, when the var is a valid JSON string,

var = "{\"a\": \"b\"}"

after quoting, it will become

"{"a": "b"}"

then, in bash

export var="{"a": "b"}"

will export the var as

{a: b}

which is no longer a valid JSON string.

Shall we use shlex.quote() instead?

@qiuosier - would you be willing to submit a PR with a suggestion for resolving this? Also do you have a use case where an envvar is a json string that we can add as a test case? Or would you be willing to open an issue for this?

[saurabhkoshatwar]

cc @loadams @tjruwase

Thanks @saurabhkoshatwar, will review. Curious on your thoughts on @qiuosier's comment?

Also can you take a look at the formatting errors?

I’m also curious about the use case where an envvar is a JSON string. @qiuosier, please share—I can update it in this PR.

[tjruwase]

I’m also curious about the use case where an envvar is a JSON string. @qiuosier, please share—I can update it in this PR.

@saurabhkoshatwar, I think it is fine for additional features to come through a separate PR. I feel this PR provides enough benefits as is.