-
Notifications
You must be signed in to change notification settings - Fork 467
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
Multiple output versions for a step outputs #3072
Multiple output versions for a step outputs #3072
Conversation
LLM Finetuning template updates in |
I have more of a general question: |
True, I didn't touch inputs here at all, since it was not in the scope of the ticket. I would prefer to handle it separately. I can do this crazy thing and I have no clue how it would be shown: @step
def step_1()->Annotated[int,"my_int"]:
return 42
@pipeline
def pipe_1():
step_1()
step_2(Client().get_artifact_version("my_int"), after=["step_1"]) So it is a Schrödinger-Artifact now: it is DEFAULT and LAZY_LOADED at the same time... |
Classification template updates in |
E2E template updates in |
I see, makes sense let's handle that separately. |
That's also why the input type can't be stored in the artifact table, one artifact might be different types of inputs for different steps/runs or even the same exact step |
…ttps://github.com/zenml-io/zenml into feature/PRD-663-multiple-output-versions-for-a-step
Yep, I also realized that during development - this will stay separately as is it is now, but we would need to enrich it for more types beyond default and manual, IMO. |
src/zenml/zen_stores/migrations/versions/1cb6477f72d6_move_artifact_save_type.py
Show resolved
Hide resolved
…ttps://github.com/zenml-io/zenml into feature/PRD-663-multiple-output-versions-for-a-step
for output_name, output_artifacts in artifacts.items(): | ||
for output_artifact in output_artifacts: | ||
artifact_config = None | ||
if output_config := output_configurations.get(output_name, None): |
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.
We need to make sure that this function is only being called with regular step output artifacts, as otherwise the artifact config does not apply
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 currently not the case
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 fixed this, but one thing we don't (and never did): If a manual save was a model artifact for the original step run, it will not be a model artifact in the cached step run.
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 pushed a test for this case, which is skipped now. We will handle this separately, as discussed.
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 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
@@ -331,7 +330,7 @@ def inputs(self) -> Dict[str, "ArtifactVersionResponse"]: | |||
return self.get_body().inputs | |||
|
|||
@property | |||
def outputs(self) -> Dict[str, "ArtifactVersionResponse"]: | |||
def outputs(self) -> Dict[str, List["ArtifactVersionResponse"]]: |
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'm also wondering whether we might want to separate this into regular outputs
from the step, and manual ones (using save_artifact
and register_artifact
)
This commit refactors the artifact saving logic in the `cacheable_multiple_versioned_producer` function. It introduces two new parameters, `is_model_artifact` and `is_deployment_artifact`, which allow specifying the type of the saved artifact. This change improves the flexibility and customization of the function. Related to #PRD-663
There is still some unrelated error on mac 3.9, but I will merge this in as is.
|
Describe changes
Highlights:
StepRunResponse.outputs
is nowDict[str, List["ArtifactVersionResponse"]]
to support multiple versions of the same artifactStepNodeDetails.outputs
is nowDict[str, List[str]]
for the same reasontype
is removed fromStepOutputSchema
and now resides in theArtifactVersionSchema
directlyDEFAULT
andMANUAL
appended with new types of artifacts:EXTERNAL
andPREEXISTING
forExternalArtifact
andregister_artifact
respectively.ArtifactVersionResponse/Request
now expectssave_type
It is very shaky for the frontend, so I would keep it for a while until we align with @Cahllagerfeld
P.S. docs to follow after some reviews feedback being collected.
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