-
Notifications
You must be signed in to change notification settings - Fork 549
Customizable Deployment server settings #4064
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
base: develop
Are you sure you want to change the base?
Conversation
dd63272
to
55719bb
Compare
45049ac
to
65c4de8
Compare
|
||
__all__ = [ | ||
"BaseStep", | ||
"ResourceSettings", |
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 can break user code and needs to be mentioned in the release notes + makes this PR breaking, is that intentional?
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 was a mistake that this was listed here and I fixed it. Nobody should import ResourceSettings from this module, given that all our docs and examples use the proper import module for this, and if they do, they will see their mistake and correct it. I can mention it in the release notes, but I won't mark this PR as breaking over such a small thing.
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 don't think it's a subjective discussion "if something is breaking or not". This import was previously part of our public API (which the steps module certainly is), and this PR removes it. So anyone that imported resource settings from here has a broken codebase after the upgrade. That makes this PR and the next release a breaking release, no matter what you or I think.
- This import was definitely not in here by mistake, because I remember when Hamza told me he wanted it there. The reason for this is because resource settings are usually defined on steps, and we wanted both to be importable from the same module for convenience (
from zenml.steps import step, ResourceSettings
).
Just FYI, I actually agree with you that the import shouldn't be here, and I'm all for removing it. Whether we do that as part of this PR or in a huge bulk PR where we do breaking changes whenever we do the next breaking release, I'll leave that up to you.
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 import was definitely not in here by mistake
I wasn't aware of this. I'll bring it back.
src/zenml/utils/source_utils.py
Outdated
self._source = resolve(self._object) | ||
return self._source | ||
|
||
def to_source_string(self) -> str: |
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.
Any reason you're serializing this to a string? The Source
object actually contains more information and is used in all other places where we serialize sources
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 don't think I understand this question. I need to serialize this to a JSON-able data type (dict, string, list, etc.). This is only used in serialization, so I can't return a Source
object here instead. It needs to be something that json.dump(...)
can work with.
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.
Maybe I'm missing something, but you're not JSON serializing this by itself, but only as an attribute of other pydantic models, correct? And Source
itself is a pydantic model, which works just fine if you use DeploymentSettings.model_dump(...)
?
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 see what you mean. I'll try it.
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.
Yep, this isn't possible because this function is already used in the PlainSerializer
serialization config and must return a JSON-friendly type.
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 reworked the entire SourceOrObject
class so that it actually extends the Source
class, which feels like a better design.
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/customizable-deployment-servers)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
Documentation Link Check Results❌ Absolute links check failed |
|
||
__all__ = [ | ||
"BaseStep", | ||
"ResourceSettings", |
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 don't think it's a subjective discussion "if something is breaking or not". This import was previously part of our public API (which the steps module certainly is), and this PR removes it. So anyone that imported resource settings from here has a broken codebase after the upgrade. That makes this PR and the next release a breaking release, no matter what you or I think.
- This import was definitely not in here by mistake, because I remember when Hamza told me he wanted it there. The reason for this is because resource settings are usually defined on steps, and we wanted both to be importable from the same module for convenience (
from zenml.steps import step, ResourceSettings
).
Just FYI, I actually agree with you that the import shouldn't be here, and I'm all for removing it. Whether we do that as part of this PR or in a huge bulk PR where we do breaking changes whenever we do the next breaking release, I'll leave that up to you.
…ll deployment settings
…le-deployment-servers
Framework-Agnostic Deployment App Factory
Overview
This PR implements a framework-agnostic deployment ASGI app factory system that allows customizing all aspects of the ASGI web application that powers up the pipeline deployments:
If necessary, the core components of the deployment server - the app factory (aka app runner) and the deployment service classes can be extended and custom implementations can be used instead of the built-in ones via configuration options.
Key Components Implemented
zenml.config.source.SourceOrObject
A hybrid type that can hold either a source string OR a loaded object:
load()
methodzenml.config.deployment_config.DeploymentSettings
Similar to
DockerSettings
, can be used to customize the configuration and behavior of the ASGI application that is run by the pipeline deployment server:SourceOrObject
to encode the location of classes and functions used as valueszenml.deployers.server.app.BaseDeploymentAppRunner
This is the abstract factory used to build and run the pipeline deployment ASGI application according to the specifications in the
DeploymentSettings
. This ASGI application is just a wrapper around the core service that implements the pipeline deployment operations (see next point). The responsibilities of this component are:DeploymentSettings
.The only built-in implementation provided for this class is the one for FastAPI:
zenml.deployers.server.fastapi.app.FastAPIDeploymentAppRunner
.Users can implement this abstract class to support any other ASGI capable framework.
zenml.deployers.server.app.BaseDeploymentAppRunnerFlavor
This implements a simple flavor system on top of
BaseDeploymentAppRunner
classes, to be able to collect software requirements for app runner implementations independently of the actual implementation.zenml.deployers.server.service.BasePipelineDeploymentService
This is the base class for the service that runs the core pipeline deployment logic. The responsibilities of this component are:
The only built-in implementation provided for this class is the one that uses the local orchestrator to run pipelines:
zenml.deployers.server.service.PipelineDeploymentService
.Users can implement this abstract class to provide their own custom implementations of running one or more pipelines.
Other Changes
secure
package to latest (1.0.1) and removes all code handling theX-XSS-Protection
security header that is no longer supportedinstall_deployment_requirements
flag to theDockerSettings
and includes deployment settings in the set of software requirements used for building container imagesContainerizedDeployer.CONTAINER_REQUIREMENTS
to theBaseDeploymentAppRunnerFlavor
classExamples
1. Basic Configuration Examples
Configure URL Paths
Configure CORS
Configure Security Headers
2. Custom Endpoints
Framework-Agnostic: Simple Endpoint Function
Framework-Agnostic: Endpoint Builder Function
FastAPI-Specific: Native FastAPI Router
3. Custom Middleware
Framework-Agnostic: Simple Middleware Function
FastAPI-Specific: Native FastAPI Middleware
4. App Extensions
Simple Extension Function
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