Add Active Stack as part of configuration YAML #2370
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent updates introduce the capability to specify an active stack for individual pipeline runs within ZenML. This is facilitated through a new attribute in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- src/zenml/config/pipeline_run_configuration.py (1 hunks)
- src/zenml/new/pipelines/pipeline.py (5 hunks)
- src/zenml/stack/utils.py (2 hunks)
- tests/integration/functional/cli/test_pipeline.py (1 hunks)
Additional comments: 5
src/zenml/config/pipeline_run_configuration.py (1)
- 32-32: The addition of
active_stack: Optional[str] = Noneto thePipelineRunConfigurationclass is correctly implemented and aligns with the PR's objective to allow specifying an active stack for pipeline runs. This change introduces flexibility in pipeline execution environments without mandating an active stack for every run.src/zenml/new/pipelines/pipeline.py (4)
- 76-76: The import of
stack_contextis correctly added to support the new feature of specifying an active stack within a pipeline's configuration YAML.- 538-540: The use of
stack_contextin thebuildmethod is correctly implemented to ensure the temporary activation of the specified stack during the pipeline build process.- 626-628: The use of
stack_contextin the_runmethod is correctly implemented to ensure the temporary activation of the specified stack during the pipeline run process.- 1159-1160: The addition of the
_update_stack_from_configmethod is correctly implemented to update the active stack from the pipeline run configuration. This method is crucial for the new feature allowing the specification of an active stack within a pipeline's configuration YAML.
| def test_pipeline_run_with_different_stack_in_config_file( | ||
| clean_client: "Client", tmp_path | ||
| ): | ||
| """Tests that the run command works with a run config file with an active stack defined.""" | ||
| runner = CliRunner() | ||
| run_command = cli.commands["pipeline"].commands["run"] | ||
|
|
||
| pipeline_id = pipeline_instance.register().id | ||
|
|
||
| components = { | ||
| key: components[0].id | ||
| for key, components in Client().active_stack_model.components.items() | ||
| } | ||
| new_stack = Client().create_stack(name="new", components=components) | ||
|
|
||
| config_path = tmp_path / "config.yaml" | ||
| run_config = PipelineRunConfiguration( | ||
| run_name="custom_run_name", active_stack=str(new_stack.id) | ||
| ) | ||
| config_path.write_text(run_config.yaml()) | ||
|
|
||
| result = runner.invoke( | ||
| run_command, [pipeline_instance.name, "--config", str(config_path)] | ||
| ) | ||
| assert result.exit_code == 0 | ||
|
|
||
| runs = Client().list_pipeline_runs(pipeline_id=pipeline_id) | ||
| assert len(runs) == 1 | ||
| assert runs[0].name == "custom_run_name" | ||
| assert runs[0].stack.id == new_stack.id | ||
| assert Client().active_stack.id != new_stack.id | ||
|
|
There was a problem hiding this comment.
The test test_pipeline_run_with_different_stack_in_config_file is well-implemented and effectively verifies that the pipeline run command correctly handles a different active stack specified in the run configuration file. This test ensures the new feature works as intended and is a valuable addition to the test suite. However, it would be beneficial to include assertions to verify that the active stack was correctly set during the pipeline run, in addition to checking the post-run conditions.
Consider enhancing the test to verify that the active stack is correctly set during the pipeline run. This could involve querying the active stack within the pipeline execution context and asserting that it matches the expected stack.
fix typo in docstring Co-authored-by: Alex Strick van Linschoten <[email protected]>
add missing return type Co-authored-by: Alex Strick van Linschoten <[email protected]>
There was a problem hiding this comment.
This is great so far but needs changes in the docs, specifically somewhere here: https://docs.zenml.io/user-guide/advanced-guide/pipelining-features/configure-steps-pipelines#breaking-configuration-down
I added the missing documentation taking the build field as a reference. |
docs/book/user-guide/advanced-guide/pipelining-features/configure-steps-pipelines.md
Outdated
Show resolved
Hide resolved
docs/book/user-guide/advanced-guide/pipelining-features/configure-steps-pipelines.md
Outdated
Show resolved
Hide resolved
…ure-steps-pipelines.md Co-authored-by: Alex Strick van Linschoten <[email protected]>
…ure-steps-pipelines.md Co-authored-by: Alex Strick van Linschoten <[email protected]>
There was a problem hiding this comment.
Thank you so much for your contribution, it's much appreciated. :)
Aside from a small bug in the __exit__ method, your implementation should work as intended. I still left a few comments to improve it however, I hope it is helpful. Let me know if you have any questions!
Sorry, I did not have time to go through all the comments this weekend. I changed to use the existing context manager. Would it make sense to move the context manager to the stack.utils module to avoid having issues with cyclic dependencies? I was also wondering why not using enter and exit instead of the yield in a try-catch block? |
|
Hey @kabinja, thank you for applying the changes. Yes, it would be great if you could move the context manager from the current file to a destination such as As for the |
|
@kabinja im so sorry for the delay here. We talked internally about this PR and it still needs some work:
What this feature should be is probably this: when a stack is specified in the yaml, it uses that stack for the pipeline run, but doesny modify your "active stack" that you've set locally:
Can you take a look at this and see if you can work on it? If not feel free to close it and we an pick it up again soon. Sorry for the staleness on our side! |
Describe changes
Issue #2261: Add Active Stack as part of configuration YAML
I added the field active_stack to PipelineRunConfiguration to be able to set up an active stack during the duration of the run. I also added a stack_context manager to allow resetting the stack at the end of the execution of the pipeline. This was done to reset the initial active stack at the end of the execution of the pipeline if one is defined by active_stack in the configuration YAML.
Pre-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
Comments
I did not find the section where the configuration file usage is referenced in the documentation.
Summary by CodeRabbit