Runtime environment variables specification #3336
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
internal
schustmi
force-pushed
the
feature/runtime-environment-variables
branch
from
8123b0e to
7876394
Compare
schustmi
force-pushed
the
feature/runtime-environment-variables
branch
from
7876394 to
9e77cbd
Compare
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def temporary_environment(environment: Dict[str, str]) -> Iterator[None]: |
There was a problem hiding this comment.
This temporary context will only affect the current process. I think users may want to set pipeline-level environment variables that will have a global effect in the environment where the pipeline steps are running. That means that these variables should be injected somehow into orchestrator.prepare_or_run_pipeline and step_operator.launch. Is this possible ?
There was a problem hiding this comment.
@stefannica That was actually my initial intention for this PR, however there are some complications: orchestrator.prepare_or_run_pipeline currently accepts a dictionary of environment variables. To make this work with our per-step configurations, we'd have to either change the public API of the orchestrator and pass different environments for all steps. Or we could expect/make it a convention that each orchestrator should update this dict with the step-specific environment configuration that is defined in the config. Or I guess we could make this a pipeline-only configuration, and don't allow setting specific environment variables for a steps, but I'm not a huge fan of this to be honest.
There was a problem hiding this comment.
WDYT, maybe the convention solution is the easiest way to go?
The orchestrator implementations would look something like this:
def prepare_or_run_pipeline(...):
for _, step in deployment.steps():
full_env = { **environment, **step.config.environment }
...There was a problem hiding this comment.
Alright, I'll adjust it then. Our local orchestrator currently ignores these environment variables. Do you think we should also set them for consistency? If yes, does that also include the auth ones that are coming from prepare_or_run_pipeline?
schustmi
force-pushed
the
feature/runtime-environment-variables
branch
from
3f06be6 to
2f79860
Compare
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/runtime-environment-variables)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
schustmi
force-pushed
the
feature/runtime-environment-variables
branch
from
21a40ad to
cae9dbc
Compare
…vironment-variables
Describe changes
This PR enables defining secrets and environment variables which will be set in the environment when executing steps of a pipeline.
Component < Stack < Pipeline < StepandEnvironment < SecretPre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes