Skip to content

Conversation

@safoinme
Copy link
Contributor

Describe changes

I implemented/fixed _ to achieve _.

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.
  • I have based my new branch on develop and the open PR is targeting develop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.
  • IMPORTANT: I made sure that my changes are reflected properly in the following resources:
    • ZenML Docs
    • Dashboard: Needs to be communicated to the frontend team.
    • Templates: Might need adjustments (that are not reflected in the template tests) in case of non-breaking changes and deprecations.
    • Projects: Depending on the version dependencies, different projects might get affected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@github-actions github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Oct 30, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

🔍 Broken Links Report

Summary

  • 📁 Files with broken links: 1
  • 🔗 Total broken links: 2
  • 📄 Broken markdown links: 2
  • 🖼️ Broken image links: 0
  • ⚠️ Broken reference placeholders: 0

Details

File Link Type Link Text Broken Path
deployers/kubernetes.md 📄 "Kubernetes service connector" ../../service-connectors/connector-types/kubernetes-cluster.md
deployers/kubernetes.md 📄 "deployment user guide" ../../user-guide/production-guide/deployment.md
📂 Full file paths
  • /home/runner/work/zenml/zenml/scripts/../docs/book/component-guide/deployers/kubernetes.md
  • /home/runner/work/zenml/zenml/scripts/../docs/book/component-guide/deployers/kubernetes.md

@safoinme safoinme requested a review from strickvl October 31, 2025 09:54
@safoinme safoinme marked this pull request as ready for review October 31, 2025 09:54
@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Documentation Link Check Results

Absolute links check passed
Relative links check failed
There are broken relative links in the documentation. See workflow logs for details
Last checked: 2025-10-31 20:52:59 UTC

Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some smaller comments / nits in there. Bigger-picture feedback:

  • feels like maybe there's room for tightening the kube_utils.py code?
  • I didn't check but is there any overlap between the K8S orchestrator and the extra kube_utils.py code? If so, we should reduce that duplication where possible.
  • probably needs tests wherever it makes sense. Feels like there's at least a bit of the logic which doesn't rely on the actual K8S infra which we should add unit tests for.

Might be more comments but these feel like the obvious ones. Mainly just about reducing duplication where it exists. A lot of those utils are just handling 404 errors and retries as far as I can see.

UPDATE: https://gist.github.com/strickvl/69f5d5e7ede6cf920b24d20ad3e83652 here are specific suggestions on that.

# Basic Kubernetes deployer configuration
settings:
docker:
parent_image: safoinme/zenml:deployer-0.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably to be removed?


settings:
docker:
parent_image: safoinme/zenml:deployer-0.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. to be removed?

zenml pipeline deploy pipelines.weather_agent.weather_agent
```

For Kubernetes deployments, use the provided configuration files:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people are going to do that then they need to set up a K8S deployer etc? So maybe link them to the docs page and tell them that they will first need to use that in their stack before this would work?

)


def wait_for_deployment_ready(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like these 'wait for...' functions have a decent amount of duplicated code. Not sure if it would make sense to refactor them to remove that?

Comment on lines +1587 to +1588
if e.status == 404:
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you do this here? (and above). Why wouldn't you want to raise the 404 as well?

If you only need local development or single-node deployments, consider the
[local](local.md) or [docker](docker.md) deployer flavors instead.

## How to deploy it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there also an 'easy' way to do this with our Terraform modules etc?

snapshots exactly like with other flavors:

```bash
zenml pipeline deploy my_module.my_pipeline --deployment k8s-example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zenml pipeline deploy my_module.my_pipeline --deployment k8s-example
zenml pipeline deploy my_module.my_pipeline --name k8s-example

--name I think and not --deployment

Comment on lines +425 to +430
### Monitoring

```bash
kubectl get hpa -n zenml-deployments --watch
kubectl describe hpa <name> -n zenml-deployments
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section feels a bit sparse... either expand it a bit more, or delete it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess people just want some guidance on how they can monitor usage etc.

@@ -0,0 +1,1737 @@
# Copyright (c) ZenML GmbH 2021. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Copyright (c) ZenML GmbH 2021. All Rights Reserved.
# Copyright (c) ZenML GmbH 2025. All Rights Reserved.

pod = self._get_pod_for_deployment(deployment)
pod_name = pod.metadata.name if pod else None

# Build metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Build metadata

Michael ☠️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request internal To filter out internal PRs and issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants