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

Add top-level artifacts: section #9220

Merged
merged 24 commits into from
Apr 5, 2023
Merged

Add top-level artifacts: section #9220

merged 24 commits into from
Apr 5, 2023

Conversation

aguschin
Copy link
Contributor

@aguschin aguschin commented Mar 21, 2023

This PR adds a top-level declarative section that is going to be used instead of GTO's artifacts.yaml
Example:

# dvc.yaml
artifacts:
  myart:
    type: model
  hello:
    type: file
    path: hello.txt

@aguschin aguschin marked this pull request as draft March 21, 2023 11:50
dvc/annotations.py Outdated Show resolved Hide resolved
dvc/dvcfile.py Outdated Show resolved Hide resolved
dvc/dvcfile.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
@daavoo daavoo added A: artifacts Related to `artifacts` section in `dvc.yaml` feature is a feature labels Mar 24, 2023
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Patch coverage: 95.60% and project coverage change: -0.01 ⚠️

Comparison is base (8be71be) 92.94% compared to head (be5101e) 92.93%.

❗ Current head be5101e differs from pull request most recent head a4180a9. Consider uploading reports for the commit a4180a9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9220      +/-   ##
==========================================
- Coverage   92.94%   92.93%   -0.01%     
==========================================
  Files         459      461       +2     
  Lines       37112    37202      +90     
  Branches     5342     5357      +15     
==========================================
+ Hits        34493    34575      +82     
- Misses       2089     2093       +4     
- Partials      530      534       +4     
Impacted Files Coverage Δ
dvc/repo/artifacts.py 88.23% <88.23%> (ø)
dvc/annotations.py 100.00% <100.00%> (ø)
dvc/dvcfile.py 96.76% <100.00%> (+0.04%) ⬆️
dvc/repo/__init__.py 94.75% <100.00%> (+0.02%) ⬆️
dvc/repo/index.py 92.75% <100.00%> (+0.06%) ⬆️
dvc/schema.py 100.00% <100.00%> (ø)
tests/func/artifacts/test_artifacts.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@aguschin aguschin marked this pull request as ready for review March 28, 2023 12:48
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/index.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/index.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
dvc/repo/artifacts.py Outdated Show resolved Hide resolved
from dvc.repo import Repo


class ArtifactsFile(FileMixin):
Copy link
Member

Choose a reason for hiding this comment

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

I'd have preferred not to use FileMixin, it's too bloated. But I am fine for now.

dvc/repo/artifacts.py Outdated Show resolved Hide resolved
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Looks good to me so far.

@dberenbaum
Copy link
Collaborator

@aguschin Let's update it to handle paths like top-level plots (use path if provided, otherwise assume ID is a path). Once you have that ready and any other cleanup, you can ping the reviewers here for a final review. Thanks!

@aguschin
Copy link
Contributor Author

aguschin commented Mar 30, 2023

Ok, ready for another review if needed :)
@dberenbaum, @skshetry @daavoo

@dberenbaum
Copy link
Collaborator

Thanks @aguschin! Could you also add monorepo tests per iterative/gto#198 (comment)?

@daavoo daavoo enabled auto-merge (squash) April 5, 2023 13:00
@daavoo daavoo merged commit 32bebc0 into iterative:main Apr 5, 2023
@aguschin aguschin deleted the artifacts branch April 5, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: artifacts Related to `artifacts` section in `dvc.yaml` feature is a feature
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants