[pipes] change env vars scheme for subprocess client #17479
Conversation
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
| env={ | ||
| **pipes_session.get_bootstrap_env_vars(), | ||
| **self.env, | ||
| **(self._base_env if self._base_env is not None else os.environ), |
There was a problem hiding this comment.
this is currently "inherit or use the specific values set on the client"
the main downside here is if you want to set some env vars at the client and inherit you have to do env={ **os.environ, 'set': 'other_vals'}
looks like for the old shell op we ended up always inheriting #6513
so some alternative options are:
- have a separate (default on) arg control whether we inherit process env vars
- just always inherit and leave it to direct
open_pipes_sessionusage to blank out
There was a problem hiding this comment.
Re:
the main downside here is if you want to set some env vars at the client and inherit you have to do env={ **os.environ, 'set': 'other_vals'}
If this were ok why not tell people just to do env=os.environ?
Unless I am confuse someone. Seems like to me we should also inherit os.environ or not and then us the "just always inherit and leave it to direct open_pipes_session usage to blank out": approach.
There was a problem hiding this comment.
I found this line of reasoning motivating to update the PR to just always inherit the parent env
alangenfeld
force-pushed
the
al/10-30-_pipes_change_env_vars_scheme_for_subprocess_client
branch
from
07a56d7 to
6eeee82
Compare
| @@ -29,8 +30,9 @@ class _PipesSubprocess(PipesClient): | |||
| a temp file, and structured messages are read from from a temp file. | |||
|
|
|||
| Args: | |||
| env (Optional[Mapping[str, str]]): An optional dict of environment variables to pass to the | |||
| subprocess. | |||
| env (Optional[Mapping[str, str]]): The base environment variables to pass to the | |||
There was a problem hiding this comment.
maybe should rename to base_env if we go with this approach
There was a problem hiding this comment.
One way to think about naming is what is the scenario where we imagine the __init__ env arg, a callsite-specific arg, and inheriting os.environ at the callsite interacting. Why use this one versus pushing everything to the callsite?
| env (Optional[Mapping[str, str]]): An optional dict of environment variables to pass to the | ||
| subprocess. | ||
| env (Optional[Mapping[str, str]]): The base environment variables to pass to the | ||
| subprocess, over layed with those set in the run call. Defaults to None, |
| context_injector=self._context_injector, | ||
| message_reader=self._message_reader, |
There was a problem hiding this comment.
not a big deal, but why change these to the private accessors?
There was a problem hiding this comment.
i was renaming env to base_env and figured that these members probably are not intentionally part of the "public" api so moved them to private to be clear.
| env={ | ||
| **pipes_session.get_bootstrap_env_vars(), | ||
| **self.env, | ||
| **(self._base_env if self._base_env is not None else os.environ), |
There was a problem hiding this comment.
Re:
the main downside here is if you want to set some env vars at the client and inherit you have to do env={ **os.environ, 'set': 'other_vals'}
If this were ok why not tell people just to do env=os.environ?
Unless I am confuse someone. Seems like to me we should also inherit os.environ or not and then us the "just always inherit and leave it to direct open_pipes_session usage to blank out": approach.
| @@ -29,8 +30,9 @@ class _PipesSubprocess(PipesClient): | |||
| a temp file, and structured messages are read from from a temp file. | |||
|
|
|||
| Args: | |||
| env (Optional[Mapping[str, str]]): An optional dict of environment variables to pass to the | |||
| subprocess. | |||
| env (Optional[Mapping[str, str]]): The base environment variables to pass to the | |||
There was a problem hiding this comment.
One way to think about naming is what is the scenario where we imagine the __init__ env arg, a callsite-specific arg, and inheriting os.environ at the callsite interacting. Why use this one versus pushing everything to the callsite?
alangenfeld
force-pushed
the
al/10-30-_pipes_change_env_vars_scheme_for_subprocess_client
branch
from
6eeee82 to
dcc8835
Compare
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit dcc8835. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit dcc8835. |
| @@ -101,9 +102,10 @@ def run( | |||
| command, | |||
| cwd=cwd or self.cwd, | |||
| env={ | |||
| **pipes_session.get_bootstrap_env_vars(), | |||
| **os.environ, | |||
There was a problem hiding this comment.
if we get user demand we can pretty easily add a bool argument to disable this if we want
|
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-6o33gaclj-elementl.vercel.app Direct link to changed pages: |
alangenfeld
deleted the
al/10-30-_pipes_change_env_vars_scheme_for_subprocess_client
branch
Update the subprocess pipes client to inherit the parent environment. This
resolves #17364
How I Tested These Changes
added test