Add API to add artifacts #9345

aguschin · 2023-04-19T12:22:42Z

WIP

~~implement dvc get artifacts~~
add writing new artifacts (used by Studio, DVCLive uses its own way due to simplicity)
Remove some name limitations: "Do you know if using a number as the first character is problematic? If not, could we drop the restriction for The first character must be a letter.? The rest of the logic seems pretty straightforward."
~~assert that get is implemented right~~
- ~~assert it's alright to simply split by : to get dvc.yaml location and artifact name~~
~~implement same logic for import~~
add tests

dberenbaum · 2023-04-19T17:37:13Z

@aguschin Converted to draft since it's WIP. Hope that's okay.

dvc/dvcfile.py

dvc/repo/get.py

dvc/commands/get.py

dvc/dvcfile.py

dvc/repo/artifacts.py

dvc/repo/get.py

dvc/commands/get.py

Co-authored-by: Oded Messer <[email protected]>

dberenbaum · 2023-04-28T12:18:28Z

@aguschin Is this still in draft mode? Is it ready to be reviewed and merged?

dberenbaum · 2023-05-02T14:08:12Z

@aguschin Let's not include in this PR, but what about dvc get-url and API methods (dvc.api.open/read and DVCFileSystem)?

skshetry · 2023-05-02T14:49:41Z

@aguschin Let's not include in this PR, but what about dvc get-url and API methods (dvc.api.open/read and DVCFileSystem)?

How would DVCFileSystem implement this? On other APIs, it feels too early.

Also, I am not a fan of overloading target here, I'd have preferred it to be unambiguous with --model or similar flags. Or, alternatively get rid of dvc.yaml path and assume model names are going to be unique, as there's a way to download using the path unambiguously.

To be honest, I am not seeing much value with this, that's not already possible.

aguschin · 2023-05-02T16:08:38Z

@aguschin Let's not include in this PR, but what about dvc get-url and API methods (dvc.api.open/read and DVCFileSystem)?

I don't think we should implement it there for the first release - don't see clearly the scenario when it's really needed when there is get/import. If users will need that, we can add it later.

Also, I am not a fan of overloading target here, I'd have preferred it to be unambiguous with --model or similar flags

Thank you for sharing your thoughts @skshetry! I understand you here. My thoughts: we use the same to reference stages in dvc.yaml, so I think it's OK to reuse same approach, since we already took it.

Or, alternatively get rid of dvc.yaml path and assume model names are going to be unique, as there's a way to download using the path unambiguously.

Rn, names must be unique within a selected dvc.yaml, but I assume you meant they should be unique over all dvc.yaml files in DVC repo? This is quite hard to maintain I think, so don't really see this as an option.

To be honest, I am not seeing much value with this, that's not already possible.

Sorry, didn't get you here. Did you meant this PR, or dvc get-url and API methods (dvc.api.open/read and DVCFileSystem)?

dberenbaum · 2023-05-02T17:41:33Z

@aguschin How will users get the revisions they want? For example, I may always want the latest prod version. I think this is more important than supporting the artifact name.

skshetry · 2023-05-03T04:14:20Z

Rn, names must be unique within a selected dvc.yaml, but I assume you meant they should be unique over all dvc.yaml files in DVC repo? This is quite hard to maintain I think, so don't really see this as an option.

It's okay to make an assumption that they are going to be unique on a repo-level without enforcing it, as long as they are documented. There's an alternative way, from path to download artifacts.

skshetry · 2023-05-03T04:20:48Z

Sorry, didn't get you here. Did you meant this PR, or dvc get-url and API methods (dvc.api.open/read and DVCFileSystem)?

I meant dvc get. Users can already download using path. We are only making it more confusing by overloading it.

I don't see much difference between either of the following options. Most of the time, the artifact will refer to the same path, they will rarely change.

$ dvc get https://github.com/iterative/example-get-started dvc.yaml:artifact1
$ dvc get https://github.com/iterative/example-get-started path/model

I do see in #9100 that you proposed:

$ dvc get $REPO mymodel@latest

Which I do see value in.

dberenbaum · 2023-05-03T16:34:35Z

$ dvc get $REPO mymodel@latest

@skshetry Agreed that this is looks more valuable, but I think it makes sense to merge support for artifact names and follow up later with gto revision support.

@aguschin Can we have some kind of revision support in dvc get for release? I think it's important since otherwise we are still stuck asking users to install and learn gto to deploy.

skshetry · 2023-05-03T16:48:41Z

Still think of either using a flag or at least getting rid of dvc.yaml path. Requiring too many information is counter-productive especially when the difference is already very minimal between path and name. It’s a remote resource that we are talking about, figuring out where the artifact is defined is even harder.

…get-artifacts

codecov · 2023-05-07T09:30:33Z

Codecov Report

Patch coverage: 93.24% and project coverage change: -0.25 ⚠️

Comparison is base (915dd50) 91.60% compared to head (11f51a6) 91.35%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9345      +/-   ##
==========================================
- Coverage   91.60%   91.35%   -0.25%     
==========================================
  Files         488      488              
  Lines       38101    38148      +47     
  Branches     5464     5469       +5     
==========================================
- Hits        34901    34852      -49     
- Misses       2637     2712      +75     
- Partials      563      584      +21

Impacted Files	Coverage Δ
dvc/schema.py	`100.00% <ø> (ø)`
dvc/repo/artifacts.py	`89.09% <85.71%> (+0.85%)`	⬆️
dvc/annotations.py	`100.00% <100.00%> (ø)`
tests/func/artifacts/test_artifacts.py	`100.00% <100.00%> (ø)`

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

omesser · 2023-05-07T13:07:58Z

dvc/annotations.py

@@ -28,9 +28,27 @@ def to_dict(self) -> Dict[str, str]:


 @dataclass
-class Artifact(Annotation):
+class Artifact:


What was the problem inheriting from Annotation class here (and just adding path: str)?

path is required, thus it should come before all other arguments. Thus simply inheriting doesn't work - it throws errors at running. I didn't find a better solution than simply copy-pasting this code unfortunately.

I don't know if there's a nicer solution here.
@skshetry - maybe you have an idea?

dvc/repo/artifacts.py

tests/func/artifacts/test_artifacts.py

dvc/repo/artifacts.py

dvc/annotations.py

dvc/repo/artifacts.py

Co-authored-by: Oded Messer <[email protected]>

Dvc get artifacts upd

aguschin · 2023-05-11T15:55:45Z

Updated to your feedback @omesser @skshetry. Looking forward to get another round here and get this merged! 🎉

dvc/repo/artifacts.py

skshetry · 2023-05-12T06:34:02Z

dvc/repo/artifacts.py

-
-    def merge(self, ancestor, other, allowed=None):
-        raise NotImplementedError
+def check_for_nested_dvc_repo(dvcfile: Path):


What's the harm in allowing this?

It's confusing, since if you add it like this, you later won't find it (it's out of scope for this DVC repo). Since Studio is the only user for this API, for now if the user adds the model like this (if we disable check), he won't find it in Studio MR. Makes sense to make this check on DVC side to me.

As i have said before, dvc does not support nested subrepos, so it does not make sense to add an error message about this.

dvc/repo/artifacts.py

aguschin added 5 commits April 7, 2023 17:46

api call to add/write artifact

697f08a

return dvcyaml path on add

8b180ca

create dir for dvc.yaml if needed

7ea0523

create dir for dvc.yaml if needed - fix

07b5e34

allow nested artifact names

caf9910

aguschin self-assigned this Apr 19, 2023

aguschin added the A: artifacts Related to `artifacts` section in `dvc.yaml` label Apr 19, 2023

aguschin mentioned this pull request Apr 19, 2023

Introduce artifacts: section in DVC and make it work with GTO #9219

Closed

20 tasks

dberenbaum marked this pull request as draft April 19, 2023 17:36

Merge branch 'main' into dvc-get-artifacts

73dbf65

aguschin mentioned this pull request Apr 25, 2023

Adding artifacts section to docs iterative/dvc.org#4481

Merged

make dvc get download artifacts

943c169

aguschin commented Apr 25, 2023

View reviewed changes

dvc/dvcfile.py Outdated Show resolved Hide resolved

aguschin commented Apr 25, 2023

View reviewed changes

dvc/repo/get.py Outdated Show resolved Hide resolved

aguschin requested a review from skshetry April 25, 2023 10:01

omesser suggested changes Apr 25, 2023

View reviewed changes

aguschin commented Apr 26, 2023

View reviewed changes

dvc/commands/get.py Outdated Show resolved Hide resolved

aguschin and others added 2 commits April 26, 2023 09:53

Apply suggestions from code review

d43302f

Co-authored-by: Oded Messer <[email protected]>

use feedback

2341322

merge main

2c370f7

aguschin added 2 commits May 7, 2023 12:19

fix parsing artifact name

448f640

Merge branch 'dvc-get-artifacts' of github.com:aguschin/dvc into dvc-…

5e09440

…get-artifacts

remove 'parse_name' and write tests for 'add'

d938fb1

omesser self-requested a review May 7, 2023 12:59

omesser suggested changes May 7, 2023

View reviewed changes

aguschin changed the title ~~dvc get artifacts~~ Add API to add artifacts May 8, 2023

skshetry reviewed May 9, 2023

View reviewed changes

dvc/annotations.py Outdated Show resolved Hide resolved

skshetry reviewed May 9, 2023

View reviewed changes

dvc/repo/artifacts.py Outdated Show resolved Hide resolved

skshetry reviewed May 9, 2023

View reviewed changes

dvc/repo/artifacts.py Outdated Show resolved Hide resolved

aguschin and others added 6 commits May 11, 2023 12:32

Apply suggestions from code review

4e437da

Co-authored-by: Oded Messer <[email protected]>

updates from comments

19f469e

fix for studio

2685e49

fix again

fbcda7f

Merge pull request #1 from aguschin/dvc-get-artifacts-upd

554ac77

Dvc get artifacts upd

Merge branch 'main' into dvc-get-artifacts

72c7465

skshetry reviewed May 12, 2023

View reviewed changes

dvc/repo/artifacts.py Outdated Show resolved Hide resolved

skshetry reviewed May 12, 2023

View reviewed changes

dvc/repo/artifacts.py Outdated Show resolved Hide resolved

skshetry reviewed May 12, 2023

View reviewed changes

making check_name_format to raise an exception

b289fb6

omesser approved these changes May 12, 2023

View reviewed changes

Merge branch 'main' into dvc-get-artifacts

12ef95d

skshetry reviewed May 15, 2023

View reviewed changes

dvc/repo/artifacts.py Outdated Show resolved Hide resolved

skshetry approved these changes May 15, 2023

View reviewed changes

Update dvc/repo/artifacts.py

11f51a6

aguschin enabled auto-merge (squash) May 15, 2023 07:30

aguschin merged commit 4518150 into iterative:main May 15, 2023

This was referenced May 23, 2023

Model registry docs iterative/dvc.org#4423

Closed

Download models (type: model) with dvc get #9100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API to add artifacts #9345

Add API to add artifacts #9345

aguschin commented Apr 19, 2023 •

edited

Loading

dberenbaum commented Apr 19, 2023

dberenbaum commented Apr 28, 2023

dberenbaum commented May 2, 2023

skshetry commented May 2, 2023

aguschin commented May 2, 2023 •

edited

Loading

dberenbaum commented May 2, 2023

skshetry commented May 3, 2023

skshetry commented May 3, 2023

dberenbaum commented May 3, 2023

skshetry commented May 3, 2023

codecov bot commented May 7, 2023 •

edited

Loading

omesser May 7, 2023

aguschin May 8, 2023

omesser May 12, 2023

aguschin commented May 11, 2023

skshetry May 12, 2023

aguschin May 12, 2023

skshetry May 15, 2023 •

edited

Loading

Add API to add artifacts #9345

Add API to add artifacts #9345

Conversation

aguschin commented Apr 19, 2023 • edited Loading

dberenbaum commented Apr 19, 2023

dberenbaum commented Apr 28, 2023

dberenbaum commented May 2, 2023

skshetry commented May 2, 2023

aguschin commented May 2, 2023 • edited Loading

dberenbaum commented May 2, 2023

skshetry commented May 3, 2023

skshetry commented May 3, 2023

dberenbaum commented May 3, 2023

skshetry commented May 3, 2023

codecov bot commented May 7, 2023 • edited Loading

Codecov Report

omesser May 7, 2023

Choose a reason for hiding this comment

aguschin May 8, 2023

Choose a reason for hiding this comment

omesser May 12, 2023

Choose a reason for hiding this comment

aguschin commented May 11, 2023

skshetry May 12, 2023

Choose a reason for hiding this comment

aguschin May 12, 2023

Choose a reason for hiding this comment

skshetry May 15, 2023 • edited Loading

Choose a reason for hiding this comment

aguschin commented Apr 19, 2023 •

edited

Loading

aguschin commented May 2, 2023 •

edited

Loading

codecov bot commented May 7, 2023 •

edited

Loading

skshetry May 15, 2023 •

edited

Loading