Skip to content
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

artifact link #1363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

artifact link #1363

wants to merge 2 commits into from

Conversation

dafnapension
Copy link
Collaborator

@dafnapension dafnapension commented Nov 18, 2024

closes: #1341

@elronbandel
Copy link
Member

elronbandel commented Nov 18, 2024

It looks good, just not sure, how to support the overwrite args.
Not sure how we could pass the arguments from the old artifact to the new one.
Lets say someone uses metrics.accurcay[lower_case=True] and metrics.accuracy points to its new name metrics.exact_match how can we pass the arguments [lower_case=True] to the new artifact ? (effectively resulting in metrics.exact_match[lower_case=True]) @yoavkatz @dafnapension what do you think?

@dafnapension
Copy link
Collaborator Author

Hi @elronbandel ,
As it is now, the new_artifact is a whole: an instantiated artifact. It is returned as is. Potentially, it does not have at all the fields specified in the overwrites of the original artifact referenced.
I think that a clear documentation can cover this issue.. :-)

@elronbandel
Copy link
Member

The issue is old code with references to deprecated catalog names.
If someone has metrics.accurcay[lower_case=True] and we referred metrics.accurcay->metrics.exact_match his code wont work as before due to the arguments overwrite not being passed: [lower_case=True]

@@ -412,6 +412,18 @@ def get_raw(obj):
return shallow_copy(obj)


class ArtifactLink(Artifact):
new_artifact: Artifact
is_self_deprecated: bool
Copy link
Member

Choose a reason for hiding this comment

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

In think deprecation and linking are distinct properties.

We can deprecate something without having a new link to it. So every artifact should have a deprecated field.

@dafnapension
Copy link
Collaborator Author

@yoavkatz , @elronbandel , I tried to address both your valuable comments. Please see.

Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a way to rename and deprecate artifacts
3 participants