artifact link #1363

dafnapension · 2024-11-18T10:24:04Z

closes: #1341

elronbandel · 2024-11-18T10:42:41Z

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 · 2024-11-18T11:16:46Z

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 · 2024-11-18T13:59:07Z

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]

yoavkatz · 2024-11-18T14:33:38Z

src/unitxt/artifact.py

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


+class ArtifactLink(Artifact):
+    new_artifact: Artifact
+    is_self_deprecated: bool


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 · 2024-11-18T22:39:25Z

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

yoavkatz · 2024-11-19T09:19:00Z

src/unitxt/artifact.py

@@ -138,6 +139,14 @@ class Artifact(Dataclass):
    )
    __id__: str = InternalField(default=None, required=False, also_positional=False)

+    __is_deprecated__: bool = NonPositionalField(


We should add description of the features to the

https://www.unitxt.ai/en/latest/docs/saving_and_loading_from_catalog.html

@yoavkatz , I removed field __is_deprecated__ altogether, since we already have the @deprecation decorator from deprecation_utils. Decorating class RenameFields, for example.
Yet, I added a test that combines deprecation with links.
And moved the link to the other Artifact to class ArtifactLink as you suggested.

Thanks Dafna.
There is a difference between deprecating a class and deprecating an instance.
For example, I can have a card saved as "card.sst" and at some point I want to call it "card.sst2".
I want to make the change be backward compatible, so "cards.sst" becomes a deprecated with warning. It is also replaced with ArtficactLink to "card.sst2".

Note that "Card" class was never deprecated

I also think a link should always be to acatalog entry and not an object.

Thank you, Yoav,
I hope I got it better now.. The easy thing was only allowing a link to be expressed as a catalog-entry id.
What I was not sure about is what to do with the 'deprecated bit' that is now back into Artifact's fields.
For now, I implemented the bit as a string | None. I.e.: if None then not deprecated, and if string - is deprecated. In such a case, a warning message, being that string, is logged as a Warn, when the artifact's prepare(self) is run. That's the only sanction..

src/unitxt/artifact.py

tests/library/test_artifact.py

src/unitxt/artifact.py

yoavkatz · 2024-11-21T12:24:39Z

src/unitxt/artifact.py

-        return Artifact.load(artifact_rep), None
+        artifact_to_return = Artifact.load(artifact_rep)
+        if isinstance(artifact_rep, ArtifactLink):
+            artifact_to_return = artifact_to_return.artifact_linked_to


Same here. The fetch_artificat should return the artificat, not the name of the artifact we link to.

Hi @yoavkatz , and thanks again for your neat idea about the dedicated load of ArtifactLink. I just expanded it to also allow link to link to link (imagine that.. with the overwrites that @elronbandel emphasized floating all the way over the links in the middle, until they can be applied on the final, non-link Artifact that can digest them - only it is expected to have these keys that show in the overwrites).

And thanks for your new comment. I see what you mean, and hope I put it right now, backed with many tests. Please see, and suggest any test you see fit.

Hi @yoavkatz , I also added the requested documentation:

Thanks! I've added some followup feedback.

src/unitxt/artifact.py

Signed-off-by: dafnapension <[email protected]>

…y to the final target artifact, which is the only one that can digest them. Signed-off-by: dafnapension <[email protected]>

…he target of the link Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

Signed-off-by: Yoav Katz <[email protected]>

Added example of links in catalog. Signed-off-by: Yoav Katz <[email protected]>

Signed-off-by: Yoav Katz <[email protected]>

Signed-off-by: dafnapension <[email protected]>

dafnapension · 2024-11-25T13:08:26Z

Hi @yoavkatz , I fought the red color, but I think I won. please let me know if this is anywhere near what you had in mind. The deprecation msg is printed at the top of the artifact documentation.
I am not sure about what you said re. artifact.__description__ . these are always printed at the top. E.g.:
https://www.unitxt.ai/en/latest/catalog/catalog.cards.almost_evil.html

which also shows on the dir level:

yoavkatz · 2024-11-25T14:34:08Z

Thanks Dafna.

Looking good. Two suggestions:

"Deprecation message:" as a prefix to the deprecation message.
Remove the deprecation message from the displayed yaml (as it is redundant)

dafnapension · 2024-11-25T15:38:35Z

Hi @yoavkatz , how's this: ?

Please point for me where you see the __description__ or __tags__ field in the yaml. I may be looking another way.. Are you referring to the Explanations about that show below the yaml, which bring the docstrings of the classes mentioned in the yaml (and were recently made by @elronbandel into clickable links)?

…side the yanl Signed-off-by: dafnapension <[email protected]>

Signed-off-by: dafnapension <[email protected]>

docs/docs/saving_and_loading_from_catalog.rst

yoavkatz · 2024-11-27T10:17:17Z

Thanks for the great work. Once it finishes testing , I'll merge it.

* artifact link Signed-off-by: dafnapension <[email protected]> * riding on recursive_load to allow overwrites with links Signed-off-by: dafnapension <[email protected]> * prepare for replacement sitting in a different catalog Signed-off-by: dafnapension <[email protected]> * removed __is_deprecated__ and moved the link to a dedicated class ArtifactLink Signed-off-by: dafnapension <[email protected]> * link only via catalog-entry id, deprecation msg for individual artifacts, not only classes Signed-off-by: dafnapension <[email protected]> * added a dedicated load() for ArtifactLink, per yoav's suggestion Signed-off-by: dafnapension <[email protected]> * a cleaner separation Signed-off-by: dafnapension <[email protected]> * allow link to link to link.. with the overwriting floating all the way to the final target artifact, which is the only one that can digest them. Signed-off-by: dafnapension <[email protected]> * test ultimate instantiation of ArtifactLink to an Actual Artifact - the target of the link Signed-off-by: dafnapension <[email protected]> * artifact_linked_to is a required field Signed-off-by: dafnapension <[email protected]> * added documentation to Tutorials Signed-off-by: dafnapension <[email protected]> * reworded documentation some Signed-off-by: dafnapension <[email protected]> * check validity of artifact_linked_to before fetching by it Signed-off-by: dafnapension <[email protected]> * Added example of deprecation of an asset Signed-off-by: Yoav Katz <[email protected]> * Made link a position field in ArtificatLink. Added example of links in catalog. Signed-off-by: Yoav Katz <[email protected]> * Fixed typo. Signed-off-by: Yoav Katz <[email protected]> * rephrased documentation per Yoav's suggestions, and added a test Signed-off-by: dafnapension <[email protected]> * made explanation rst-friendly Signed-off-by: dafnapension <[email protected]> * some suggested changes I missed before Signed-off-by: dafnapension <[email protected]> * red deprecation Signed-off-by: dafnapension <[email protected]> * show deprecation message only at the top of the documentation, not inside the yanl Signed-off-by: dafnapension <[email protected]> * also leave the existing removal of __tags__ and __description__ Signed-off-by: dafnapension <[email protected]> * deprecated regular asset Signed-off-by: dafnapension <[email protected]> --------- Signed-off-by: dafnapension <[email protected]> Signed-off-by: Yoav Katz <[email protected]> Co-authored-by: Yoav Katz <[email protected]>

dafnapension requested a review from elronbandel November 18, 2024 10:25

dafnapension force-pushed the artifact_link branch from a90b2e2 to f2716f1 Compare November 18, 2024 10:25

yoavkatz reviewed Nov 18, 2024

View reviewed changes

dafnapension force-pushed the artifact_link branch from 2878941 to 871ebaf Compare November 18, 2024 17:25

dafnapension force-pushed the artifact_link branch 2 times, most recently from 306bc37 to 09b6611 Compare November 19, 2024 08:32

yoavkatz reviewed Nov 19, 2024

View reviewed changes

src/unitxt/artifact.py Outdated Show resolved Hide resolved

yoavkatz reviewed Nov 19, 2024

View reviewed changes

tests/library/test_artifact.py Outdated Show resolved Hide resolved

dafnapension force-pushed the artifact_link branch 7 times, most recently from 22781e2 to 8a2cfb9 Compare November 20, 2024 14:12

yoavkatz reviewed Nov 20, 2024

View reviewed changes

src/unitxt/artifact.py Show resolved Hide resolved

dafnapension force-pushed the artifact_link branch 3 times, most recently from c3931a7 to 33fc54a Compare November 21, 2024 12:16

yoavkatz reviewed Nov 21, 2024

View reviewed changes

src/unitxt/artifact.py Show resolved Hide resolved

yoavkatz reviewed Nov 21, 2024

View reviewed changes

dafnapension force-pushed the artifact_link branch from 97ce658 to 86f2dd1 Compare November 21, 2024 15:40

yoavkatz reviewed Nov 21, 2024

View reviewed changes

src/unitxt/artifact.py Outdated Show resolved Hide resolved

dafnapension force-pushed the artifact_link branch 2 times, most recently from 4676b32 to 117080d Compare November 22, 2024 22:45

dafnapension and others added 14 commits November 25, 2024 15:05

added a dedicated load() for ArtifactLink, per yoav's suggestion

4124d67

Signed-off-by: dafnapension <[email protected]>

a cleaner separation

682cd5f

Signed-off-by: dafnapension <[email protected]>

allow link to link to link.. with the overwriting floating all the wa…

9d28b8f

…y to the final target artifact, which is the only one that can digest them. Signed-off-by: dafnapension <[email protected]>

test ultimate instantiation of ArtifactLink to an Actual Artifact - t…

f924efc

…he target of the link Signed-off-by: dafnapension <[email protected]>

artifact_linked_to is a required field

e371d01

Signed-off-by: dafnapension <[email protected]>

added documentation to Tutorials

b49450c

Signed-off-by: dafnapension <[email protected]>

reworded documentation some

fb2a7d3

Signed-off-by: dafnapension <[email protected]>

check validity of artifact_linked_to before fetching by it

e6eddbd

Signed-off-by: dafnapension <[email protected]>

Added example of deprecation of an asset

da967c9

Signed-off-by: Yoav Katz <[email protected]>

Made link a position field in ArtificatLink.

964f3d9

Added example of links in catalog. Signed-off-by: Yoav Katz <[email protected]>

Fixed typo.

2050a62

Signed-off-by: Yoav Katz <[email protected]>

rephrased documentation per Yoav's suggestions, and added a test

f7f8d5e

Signed-off-by: dafnapension <[email protected]>

made explanation rst-friendly

210f195

Signed-off-by: dafnapension <[email protected]>

some suggested changes I missed before

3a95494

Signed-off-by: dafnapension <[email protected]>

dafnapension force-pushed the artifact_link branch from 89b3bcf to 3a95494 Compare November 25, 2024 13:05

red deprecation

762a7fa

Signed-off-by: dafnapension <[email protected]>

dafnapension added 3 commits November 25, 2024 17:41

show deprecation message only at the top of the documentation, not in…

716647d

…side the yanl Signed-off-by: dafnapension <[email protected]>

also leave the existing removal of __tags__ and __description__

2719464

Signed-off-by: dafnapension <[email protected]>

deprecated regular asset

14c31b5

Signed-off-by: dafnapension <[email protected]>

yoavkatz approved these changes Nov 27, 2024

View reviewed changes

docs/docs/saving_and_loading_from_catalog.rst Show resolved Hide resolved

Merge branch 'main' into artifact_link

e21524b

yoavkatz enabled auto-merge (squash) November 27, 2024 10:16

yoavkatz merged commit f203f41 into main Nov 27, 2024
16 checks passed

yoavkatz deleted the artifact_link branch November 27, 2024 10:24

yoavkatz mentioned this pull request Nov 27, 2024

Additional changes to ArtifactLink and Deprecations #1396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

artifact link #1363

artifact link #1363

dafnapension commented Nov 18, 2024 •

edited

Loading

elronbandel commented Nov 18, 2024 •

edited

Loading

dafnapension commented Nov 18, 2024

elronbandel commented Nov 18, 2024

yoavkatz Nov 18, 2024

dafnapension commented Nov 18, 2024

yoavkatz Nov 19, 2024

dafnapension Nov 19, 2024 •

edited

Loading

yoavkatz Nov 20, 2024

dafnapension Nov 20, 2024

yoavkatz Nov 21, 2024

dafnapension Nov 21, 2024 •

edited

Loading

dafnapension Nov 21, 2024 •

edited

Loading

yoavkatz Nov 24, 2024

dafnapension commented Nov 25, 2024 •

edited

Loading

yoavkatz commented Nov 25, 2024 •

edited

Loading

dafnapension commented Nov 25, 2024

yoavkatz commented Nov 27, 2024

artifact link #1363

artifact link #1363

Conversation

dafnapension commented Nov 18, 2024 • edited Loading

elronbandel commented Nov 18, 2024 • edited Loading

dafnapension commented Nov 18, 2024

elronbandel commented Nov 18, 2024

yoavkatz Nov 18, 2024

Choose a reason for hiding this comment

dafnapension commented Nov 18, 2024

yoavkatz Nov 19, 2024

Choose a reason for hiding this comment

dafnapension Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

yoavkatz Nov 20, 2024

Choose a reason for hiding this comment

dafnapension Nov 20, 2024

Choose a reason for hiding this comment

yoavkatz Nov 21, 2024

Choose a reason for hiding this comment

dafnapension Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

dafnapension Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

yoavkatz Nov 24, 2024

Choose a reason for hiding this comment

dafnapension commented Nov 25, 2024 • edited Loading

yoavkatz commented Nov 25, 2024 • edited Loading

dafnapension commented Nov 25, 2024

yoavkatz commented Nov 27, 2024

dafnapension commented Nov 18, 2024 •

edited

Loading

elronbandel commented Nov 18, 2024 •

edited

Loading

dafnapension Nov 19, 2024 •

edited

Loading

dafnapension Nov 21, 2024 •

edited

Loading

dafnapension Nov 21, 2024 •

edited

Loading

dafnapension commented Nov 25, 2024 •

edited

Loading

yoavkatz commented Nov 25, 2024 •

edited

Loading