-
Notifications
You must be signed in to change notification settings - Fork 536
Store client-side pipeline run logs in the artifact store #3498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:
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
|
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/pipeline-run-logs)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
@@ -243,6 +249,10 @@ class PipelineRunResponseMetadata(ProjectScopedResponseMetadata): | |||
title="Substitutions used in the step runs of this pipeline run.", | |||
default_factory=dict, | |||
) | |||
logs: Optional["LogsResponse"] = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're newly introducing this, I think PipelineRunResponseResources
would be the appropriate container for this attribute, as this refers to another response model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
compiled_deployment, schedule, build = self._compile( | ||
**self._run_args | ||
) | ||
self._run_args["deployment"] = compiled_deployment | ||
self._run_args["schedule"] = schedule | ||
self._run_args["build"] = build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super ugly 😅 I'm guessing this was necessary to figure out the final state of enable_pipeline_logs
before calling the rest of the code?
I'm wondering if there's a cleaner way to achieve this? I guess the value is just:
logging_enabled = run_config.enable_pipeline_logs
if logging_enabled is None:
logging_enabled = self.config.enable_pipeline_logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just super-ugly, but apparently also breaking everything.
Your suggestion looks a lot better. Done.
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 24.4%, saving 74.06 KB.
497 images did not require optimisation. Update required: Update image-actions configuration to the latest version before 1/1/21. See README for instructions. |
logging_enabled = False | ||
else: | ||
logging_enabled = self._run_args.get( | ||
"enable_pipeline_logs", True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still missing the check of self.config.enable_pipeline_logs
as a last fallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I missed that. Done.
Documentation Link Check Results✅ Absolute links check passed |
Document the new feature from PR #3498 that stores client-side pipeline logs in the artifact store, making them accessible after client session ends. Update configuration options and add benefits section. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
* Add deployment link * Move Custom Output Names section higher in docs to highlight fundamental concept 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Fix images in self-hosted page * Reorg IaC section and move best practices to 'Learn' tab * Fix TOC * Clarify OSS vs Pro features in Model Control Plane documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Refine wording in Model Control Plane documentation to enhance clarity regarding open-source and Pro features * Fix GIF URL * Clarify Workspace URL meaning for Pro users in Terraform docs Add specific guidance for ZenML Pro users about what the server_url parameter means in their context, making it clear they should use their Workspace URL from the dashboard. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Clarify Workspace URL and API key usage in Terraform IaC docs Add specific information for ZenML Pro users about Workspace URL configuration and improve the API key generation documentation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Update logging docs to include client-side pipeline logs storage Document the new feature from PR #3498 that stores client-side pipeline logs in the artifact store, making them accessible after client session ends. Update configuration options and add benefits section. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Add schedule types column to orchestrator scheduling support table This commit adds a new column to the orchestrator comparison table in the scheduling documentation that specifies which schedule types (Cron, Interval, One-time) are supported by each orchestrator with scheduling capabilities. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Make the distinction between configure and with_options * Update with a gotcha * Improve the service connectors docs page * add images * Enhance Docker build documentation with details from DockerSettings docstring - Add details about the Docker build process flow - Document requirements installation order - Explain skip_build behavior in detail - Add notes about custom Dockerfile usage - Improve details on environment replication options - Add section on package installer options - Document build reuse prevention options - Add section on target repository configuration - Add section on container user permissions - Add new best practice about container security 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Remove the prevent build reuse stuff * Remove unused terraform-stack-management.md file * Fix merge conflict remaining markers * Add redirect for IaC page * Add workspace URL image * Remove bad redirect * Move the image outside the info box * Testing * add a warning hint about logs --------- Co-authored-by: Claude <[email protected]>
Describe changes
Reuse the existing step logger context and the pipeline run logs reference to collect the client-side stdout and stderr output of a pipeline run and store it in the active artifact store, so it can be fetched and displayed in the pipeline run logs tab in the dashboard.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes