Skip to content

Conversation

@Json-Andriopoulos
Copy link
Contributor

@Json-Andriopoulos Json-Andriopoulos commented Oct 7, 2025

  • Allow to specify multiple entities in same log_metadata() call
  • Batch submit RunMetadataResource objects
  • Allow to attach metadata for multiple artifacts with infer
  • Split RunMetadataResource resolution logic for testability

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 the enhancement New feature or request label Oct 7, 2025
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch 4 times, most recently from 6784dcb to 67dfc61 Compare October 7, 2025 12:17
@Json-Andriopoulos Json-Andriopoulos marked this pull request as ready for review October 8, 2025 08:45
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch from a1ac646 to f65865f Compare October 17, 2025 07:09
@bcdurak bcdurak linked an issue Oct 17, 2025 that may be closed by this pull request
1 task
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch 4 times, most recently from 5f1c747 to 24e675a Compare October 21, 2025 07:00
Copy link
Contributor

@bcdurak bcdurak left a comment

Choose a reason for hiding this comment

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

Leaving a few small notes but overall the changes look good. We might need some documentation to reflect the new changes though.

Comment on lines +393 to +395
infer_models: Flag - when enabled infer model to log metadata for from step context.
infer_artifacts: Flag - when enabled infer artifact to log metadata for from step context.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering whether these could be a part of the respective VersionedIdentifiers. In the current version, it is impossible to log metadata for an output artifact of the step and an external artifact at the same time.

Copy link
Contributor Author

@Json-Andriopoulos Json-Andriopoulos Oct 21, 2025

Choose a reason for hiding this comment

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

I think the distinction we do leads to better usage patterns though. Use infer within a function or explicit values out of it makes sense. Why would you log metadata for another step's artifact? Sounds a bit bug-prone.

IMO we should try to guide users to safer usage and if they are restricted by the signatures or the functionality they can always use the client directly (as per the updated docs).

Copy link
Contributor

@schustmi schustmi Oct 21, 2025

Choose a reason for hiding this comment

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

Why would you log metadata for another step's artifact

No strong opinion on whether we allow it in the bulk operations, but I've seen some usage of this before on Slack and thought I'll mention it here:
Imagine the following very simple pipeline:

@pipeline
def training_pipeline():
  train_data, test_data = load_data()
  model = train(train_data)
  eval(test_data, model)

In the eval step, you might want to attach some metadata to the artifact of the model, e.g. some metrics how it performed on the test data.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @schustmi. That was the point that I was trying to make.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on whether we change this or not, the docs might need some small updates as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got the point. Cool, will change the implementation to enable usage of both modes. This should enable users to log metadata both by infering entities from step context and using provided entities from the arguments. Just keep into account that since these 2 options are separate blocks of execution and manipulate data differently I can not easily and cleanly merge the options by moving infer into VersionedIdentifier definition.

@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch from 24e675a to 72639ec Compare October 21, 2025 11:48
@github-actions
Copy link
Contributor

github-actions bot commented Oct 21, 2025

Documentation Link Check Results

Absolute links check failed
There are broken absolute links in the documentation. See workflow logs for details
Relative links check passed
Last checked: 2025-10-23 10:18:01 UTC

@zenml-io zenml-io deleted a comment from github-actions bot Oct 21, 2025
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch from 836045c to 88eb2d3 Compare October 21, 2025 12:27
Comment on lines +393 to +395
infer_models: Flag - when enabled infer model to log metadata for from step context.
infer_artifacts: Flag - when enabled infer artifact to log metadata for from step context.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @schustmi. That was the point that I was trying to make.

Comment on lines +393 to +395
infer_models: Flag - when enabled infer model to log metadata for from step context.
infer_artifacts: Flag - when enabled infer artifact to log metadata for from step context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on whether we change this or not, the docs might need some small updates as well.

@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch from ac4d021 to e14b036 Compare October 21, 2025 15:55
Copy link
Contributor

@bcdurak bcdurak left a comment

Choose a reason for hiding this comment

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

Changes look good, I love the attention to the docs and the tests.

@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4015-multi-log-metadata branch from e14b036 to 40ff6c3 Compare October 23, 2025 09:32
@Json-Andriopoulos Json-Andriopoulos merged commit 53cc8e7 into develop Oct 23, 2025
65 of 69 checks passed
@Json-Andriopoulos Json-Andriopoulos deleted the feature/4015-multi-log-metadata branch October 23, 2025 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request run-slow-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to log metadata for multiple entries at once

4 participants