-
Couldn't load subscription status.
- Fork 549
Feature:4015 Log metadata changes #4049
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
6784dcb to
67dfc61
Compare
a1ac646 to
f65865f
Compare
5f1c747 to
24e675a
Compare
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.
Leaving a few small notes but overall the changes look good. We might need some documentation to reflect the new changes though.
| 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. |
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 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.
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 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).
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.
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.
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.
+1 to @schustmi. That was the point that I was trying to make.
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.
Depending on whether we change this or not, the docs might need some small updates as well.
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.
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.
24e675a to
72639ec
Compare
Documentation Link Check Results❌ Absolute links check failed |
836045c to
88eb2d3
Compare
| 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. |
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.
+1 to @schustmi. That was the point that I was trying to make.
| 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. |
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.
Depending on whether we change this or not, the docs might need some small updates as well.
ac4d021 to
e14b036
Compare
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.
Changes look good, I love the attention to the docs and the tests.
e14b036 to
40ff6c3
Compare
Describe changes
I implemented/fixed _ to achieve _.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes