Repository: iterative/dvc
Pull request #9345 (2023-04-19 12:22:42)

Add API to add artifacts
WIP
- [ ] ~~implement `dvc get` artifacts~~
- [x] add writing new artifacts (used by Studio, DVCLive uses its own way due to simplicity)
- [x] 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`~~
- [x] add tests


dberenbaum: @aguschin Converted to draft since it's WIP. Hope that's okay.
dberenbaum: @aguschin Is this still in draft mode? Is it ready to be reviewed and merged?
dberenbaum: @aguschin Let's not include in this PR, but what about `dvc get-url` and API methods (`dvc.api.open/read` and `DVCFileSystem`)?
skshetry: > @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: > @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 opinion @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: @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: > 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: > 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.

```console
$ 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 https://github.com/iterative/dvc/issues/9100 that you proposed:
```console
$ dvc get $REPO mymodel@latest
```

Which I do see value in.


dberenbaum: > $ 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: 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.
aguschin: > @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.

@dberenbaum, if they did stage assignments with Studio, we expose right revision to Studio's `dvc get` command, so no need to use GTO. If they don't use Studio, they did assignments with GTO, so it's ok to expect them to use GTO to get right revisions. Please correct me if I'm missing something.

> It’s a remote resource that we are talking about, figuring out where the artifact is defined is even harder.

This is a really good point, as well as the other one. WDYT @dberenbaum? 

> 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, OK, I get your point. Please help me understand then: what happens if I have same artifact names in multiple dvc.yamls across the repo? Specially, what happens when I call `dvc get $REPO mymodel`? (maybe there are some other unclear cases, please share if you see them).

And to mention, there still be cases, when knowing the path to dvc.yaml will be required. Like when you have a monorepo like https://github.com/aguschin/monorepo-test-no-root - you must supply `s3` or `ssh` there, otherwise DVC won't know it needs to check those dirs for dvc.yamls and artifacts in them.
dberenbaum: > @dberenbaum, if they did stage assignments with Studio, we expose right revision to Studio's `dvc get` command, so no need to use GTO. If they don't use Studio, they did assignments with GTO, so it's ok to expect them to use GTO to get right revisions. Please correct me if I'm missing something.

They could be doing stage assignments in Studio but deployments via some automated CD process, right? I think this is the happy path, but let me know if you have a different idea.

Ideally, the `dvc get` command they copy can be reused. The artifact name should be static, but the rev will likely change frequently.

> @skshetry, OK, I get your point. Please help me understand then: what happens if I have same artifact names in multiple dvc.yamls across the repo? Specially, what happens when I call `dvc get $REPO mymodel`? (maybe there are some other unclear cases, please share if you see them).

For `dvc get $REPO mymodel` with multiple `mymodel` artifacts across different `dvc.yaml` files, we could either fail or decide on some priority order, and we could still allow `subdir/dvc.yaml:mymodel`/`subdir/mymodel` for when you need to be more specific. We could also do `art:mymodel`/`model:mymodel` instead of `dvc.yaml:mymodel` which would be more meaningful and less like a path if needed.

Overall, I think @skshetry raises a good point that abstracting the path (which is likely static) seems less important than abstracting the revision. WDYT about focusing there instead @aguschin?
aguschin: Thanks @dberenbaum! I generally agree - my only doubt is that we need to implement it before the release.

> Ideally, the `dvc get` command they copy can be reused. The artifact name should be static, but the rev will likely change frequently

Sure. In Studio they currently they copy a command for a single _version_, not what's assigned to stage. Such command would be helpful for local/remote use, not in CI/CD, but not THAT useful in CI/CD IMHO.

> They could be doing stage assignments in Studio but deployments via some automated CD process, right? I think this is the happy path, but let me know if you have a different idea.

There are generally 2 cases: CI is triggered by a Git tag, or CI is triggered by something else.

If you run CI/CD, it's triggered by a right revision already (I promoted `mymodel` to `prod` -> that creates git tag -> at push git tag triggers CI/CD). So in this kind of CI/CD you don't need to know revision. You simply can run `dvc get . mymodel`.

In a far less frequent (my gut feeling, please correct me if you disagree) case you'd be running CI/CD deployment by schedule or when it was triggered by some external signal (webhook or whatever). In this case, you'll need to know which version is in prod. But CI/CD usually starts with a shallow cloned repo, so there are technical difficulties there we need to resolve, so it doesn't look like running `dvc get . mymodel#prod` is the last obstacle on that path:

`dvc get . mymodel --rev mymodel#prod#13` will fail with `ERROR: failed to get 'model.pkl' from '.' - unknown Git revision 'mymodel@v0.0.1'` since `mymodel#prod#13` is outside of the shallow clone. Increasing the depth of shallow clone doesn't help, that's why I have some special commands run in Gto-action to fetch all git tags https://github.com/iterative/gto-action/blob/v1/entrypoint.sh#L9-L11

(Maybe instead of fetching all git tags like this user can specify remote repo url like `dvc get $REPO_URL mymodel#prod` and that will works 🤔  - I didn't try this)

To hide all this complexity, they better use GTO action to find out right revision or even run `dvc get` inside that action (I need to update it though to accept not `$GITHUB_REF` only, but a direct instruction - like `mymodel#prod` or `mymodel@latest` - that doesn't work now). And that action extends to GH only so far.

TLDR: using `dvc get . mymodel#prod` for CI won't be that frequent (IMHO) and there is another thing(s) users need to understand, so implementing will help, but not that drastically. That's why I don't think we need to rush to implement this before the release.
dberenbaum: I was thinking of more "niche" scenarios where, for example, a separate engineering team downloads and deploys the ML team's model in a different repo (for example, as part of a daily job). I think it's pretty common for a more "enterprise" scenario, but I get why you think it's less of a priority, and I agree it's okay to drop it from the initial scope.

> If you run CI/CD, it's triggered by a right revision already (I promoted `mymodel` to `prod` -> that creates git tag -> at push git tag triggers CI/CD). So in this kind of CI/CD you don't need to know revision. You simply can run `dvc get . mymodel`.

So if this is the "happy path," even `dvc get` is optional, right? I could just as easily use `dvc pull`, although suggesting `dvc get` makes sense since it's more universal.

Do you see this PR as critical to that happy path? We will still need to explain a lot about how to setup CD to work with the artifact. Should we deprioritize this PR and focus that time on showing useful instructions in Studio and the docs explaining how to trigger the workflow and get the model?

For example, in GitHub, I guess you need something like this in your workflow file?

```yaml
on:
  push:
    tags:
      - 'mymodel#prod#[0-9]+'
...
jobs:
  deploy:
    steps:
      - uses: iterative/setup-dvc@v1
      - name: get-model
         run: |
           dvc get . path/to/mymodel
           ...
```
shcheklein: (sorry, hard to follow all the converstation) 

Some of my thoughts. I feels important to have an access MLFlow-style (`dvc get $REPO mymodel@latest` _and_ Python api that corresponds to that). Let's think why:

- simplicity, since I have name - it's easier to write, think about names as aliases, etc (vs full path that I need to remember)
- similarity with how other MRs / artifact registries work. Again, mental load of relying on both sometimes - names and path is bigger. It's something new to manage (pathes), etc
- if we keep path as an ID It means I have 3-5 places (dvc.yaml, .dvc, CI/CD, code ..) with that path hardcoded. Again, seems to be a minor things but it leads to frustration, errors, etc. Imagine a case when I do refactoring and moving an artifact. I can't now even write a CI/CD that would work both (not sure it needed?) and I would have to update the workflow file ... breaks the point of having names in the first place
- onboarding ppl into MR - also easier to "sell" here is the name and stage and you can get a model by it (related to the first item in the list)

(I'm not arguing about specific syntax here btw, overloading dvc get or make it more explicit - this is primarily about being able to address model by their names)
dberenbaum: @shcheklein @aguschin What would be your priority between these, and which do you think are blockers for release?
1. `dvc get` name support
2. `dvc get` tag support
3. Python API support
4. Import and get-url support

IMO having `dvc get` name support without `dvc get` version/tag support feels incomplete, so I would lean towards doing both (doesn't have to be in the same PR) or following up with both after release rather than doing half of it now.
shcheklein: > IMO having dvc get name support without dvc get version/tag support feels incomplete, so I would lean towards doing both (doesn't have to be in the same PR) or following up with both after release rather than doing half of it now.

sounds good to me. If we agree on the scope we can do it step by step and ship it as it's getting ready.

> Import and get-url support

Never thought about this even. But sounds also reasonable. It feels it can come "for free" if we do names support in `dvc get`. I wonder what @aguschin thinks here.
aguschin: Thank you @shcheklein @dberenbaum, this is a very helpful discussion!

> So if this is the "happy path," even dvc get is optional, right? I could just as easily use dvc pull, although suggesting dvc get makes sense since it's more universal.

Yes I think. You still need to figure out what artifact was promoted to the stage and which `path` it has, so a simple `dvc pull` may not be enough if you have 1+ models.

> Never thought about this even. But sounds also reasonable. It feels it can come "for free" if we do names support in dvc get. I wonder what @aguschin thinks here.

`import`ing artifacts can be useful. E.g. I can have a separate repo where I consume few models and build an application. I can `import` a model of a separate revision, and have CI/CD creating PRs to update versions once new versions are promoted to `prod` or `dev` (like `dependabot` does it for packages). I may even want to use not a Git revision, but what's in `prod` directly - `import`ing `mymodel@latest` or `mymodel#prod` like we discuss it to work for DVC get.

In terms of implementation, I'm not sure how involved it would be. 

> IMO having `dvc get` name support without `dvc get` version/tag support feels incomplete, so I would lean towards doing both (doesn't have to be in the same PR) or following up with both after release rather than doing half of it now.

Yeah, agree on "incompleteness" feeling here, even though I think it comes from not able to show `dvc get . mymodel@latest` - which may not be the most useful thing (as CI/CD triggered by tags is I believe), but that creates a feeling something is missing in our MR implementation. 

So I guess it comes down to this plan, sorted by priority:
1. Write docs (with CI/CD examples), release it in Studio (which means I need to merge this PR, at least the part where we can add new artifacts to `dvc.yaml`), release new versions of gto and gto-action.
2. Support `dvc get . mymodel`, `dvc get . mymodel#prod`, Python API support, update docs
3. Support `dvc import`, `dvc get-url` and etc (is there is something else), update docs

So I'm going to de-prioritize `dvc get` then, change this PR to introduce adding new artifacts only, and start drafting docs so we have at least a rough version of those.
aguschin: @omesser @skshetry may I ask you for a review? Thanks!
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) Report
Patch coverage: **`35.89`**% and project coverage change: **`-0.36`** :warning:
> Comparison is base [(`b044bcc`)](https://app.codecov.io/gh/iterative/dvc/commit/b044bcce5edbe1d8549970b1b426cb1b14efe073?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) 91.61% compared to head [(`5e09440`)](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) 91.26%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #9345      +/-   ##
==========================================
- Coverage   91.61%   91.26%   -0.36%     
==========================================
  Files         487      487              
  Lines       37778    37805      +27     
  Branches     5436     5438       +2     
==========================================
- Hits        34610    34501     -109     
- Misses       2612     2724     +112     
- Partials      556      580      +24     
```


| [Impacted Files](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) | Coverage Δ | |
|---|---|---|
| [tests/func/artifacts/test\_artifacts.py](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-dGVzdHMvZnVuYy9hcnRpZmFjdHMvdGVzdF9hcnRpZmFjdHMucHk=) | `100.00% <ø> (ø)` | |
| [dvc/repo/artifacts.py](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-ZHZjL3JlcG8vYXJ0aWZhY3RzLnB5) | `59.09% <35.89%> (-29.15%)` | :arrow_down: |

... and [38 files with indirect coverage changes](https://app.codecov.io/gh/iterative/dvc/pull/9345/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/iterative/dvc/pull/9345?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative).

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

Patch:
diff --git a/dvc/annotations.py b/dvc/annotations.py
index 6f0a7e0aa..ed7e63869 100644
--- a/dvc/annotations.py
+++ b/dvc/annotations.py
@@ -28,9 +28,21 @@ class Annotation:
 
 
 @dataclass
-class Artifact(Annotation):
+class Artifact:
     PARAM_PATH: ClassVar[str] = "path"
-    path: Optional[str] = None
+    PARAM_DESC: ClassVar[str] = "desc"
+    PARAM_TYPE: ClassVar[str] = "type"
+    PARAM_LABELS: ClassVar[str] = "labels"
+    PARAM_META: ClassVar[str] = "meta"
+
+    path: str
+    desc: Optional[str] = None
+    type: Optional[str] = None  # noqa: A003
+    labels: List[str] = field(default_factory=list)
+    meta: Dict[str, Any] = field(default_factory=dict)
+
+    def to_dict(self) -> Dict[str, str]:
+        return compact(asdict(self))
 
 
 ANNOTATION_FIELDS = [field.name for field in fields(Annotation)]
diff --git a/dvc/repo/artifacts.py b/dvc/repo/artifacts.py
index e39fa67ac..0615486c2 100644
--- a/dvc/repo/artifacts.py
+++ b/dvc/repo/artifacts.py
@@ -1,19 +1,26 @@
 import logging
-import os
 import re
-from typing import TYPE_CHECKING, Dict
+from pathlib import Path
+from typing import Dict, Optional
 
 from dvc.annotations import Artifact
-from dvc.dvcfile import FileMixin
+from dvc.dvcfile import PROJECT_FILE
+from dvc.exceptions import InvalidArgumentError
+from dvc.repo import Repo
 from dvc.utils import relpath
-
-if TYPE_CHECKING:
-    from dvc.repo import Repo
+from dvc.utils.serialize import modify_yaml
 
 logger = logging.getLogger(__name__)
 
 
-NAME_RE = re.compile(r"^[a-z]([a-z0-9-]*[a-z0-9])?$")
+# Constants are taken from GTO.
+# When we make it a dependency, we can import them instead
+SEPARATOR_IN_NAME = ":"
+DIRNAME = r"[a-z0-9-_./]+"
+NAME = r"[a-z0-9]([a-z0-9-/]*[a-z0-9])?"
+NAME_RE = re.compile(f"^{NAME}$")
+FULLNAME = f"((?P<dirname>{DIRNAME}){SEPARATOR_IN_NAME})?(?P<name>{NAME})"
+FULLNAME_RE = re.compile(f"^{FULLNAME}$")
 
 
 def name_is_compatible(name: str) -> bool:
@@ -22,22 +29,23 @@ def name_is_compatible(name: str) -> bool:
 
 def check_name_format(name: str) -> None:
     if not name_is_compatible(name):
-        logger.warning(
-            "Can't use '%s' as artifact name (ID)."
+        raise InvalidArgumentError(
+            f"Can't use '{name}' as artifact name (ID)."
             " You can use letters and numbers, and use '-' as separator"
-            " (but not at the start or end). The first character must be a letter.",
-            name,
+            " (but not at the start or end)."
         )
 
 
-class ArtifactsFile(FileMixin):
-    from dvc.schema import SINGLE_ARTIFACT_SCHEMA as SCHEMA
-
-    def dump(self, stage, **kwargs):
-        raise NotImplementedError
-
-    def merge(self, ancestor, other, allowed=None):
-        raise NotImplementedError
+def check_for_nested_dvc_repo(dvcfile: Path):
+    if dvcfile.is_absolute():
+        raise InvalidArgumentError("Use relative path to dvc.yaml.")
+    path = dvcfile.parent
+    while path.name:
+        if (path / Repo.DVC_DIR).is_dir():
+            raise InvalidArgumentError(
+                f"Nested DVC repos like {path} are not supported."
+            )
+        path = path.parent
 
 
 class Artifacts:
@@ -50,18 +58,30 @@ class Artifacts:
             dvcfile,
             dvcfile_artifacts,
         ) in self.repo.index._artifacts.items():  # pylint: disable=protected-access
-            # read the artifacts.yaml file if needed
-            if isinstance(dvcfile_artifacts, str):
-                dvcfile_artifacts = ArtifactsFile(
-                    self.repo,
-                    os.path.join(os.path.dirname(dvcfile), dvcfile_artifacts),
-                    verify=False,
-                ).load()
-            if not dvcfile_artifacts:
-                continue
             dvcyaml = relpath(dvcfile, self.repo.root_dir)
             artifacts[dvcyaml] = {}
             for name, value in dvcfile_artifacts.items():
-                check_name_format(name)
+                try:
+                    check_name_format(name)
+                except InvalidArgumentError as e:
+                    logger.warning(e.msg)
                 artifacts[dvcyaml][name] = Artifact(**value)
         return artifacts
+
+    def add(self, name: str, artifact: Artifact, dvcfile: Optional[str] = None):
+        with self.repo.scm_context(quiet=True):
+            check_name_format(name)
+            dvcyaml = Path(dvcfile or PROJECT_FILE)
+            check_for_nested_dvc_repo(
+                dvcyaml.relative_to(self.repo.root_dir)
+                if dvcyaml.is_absolute()
+                else dvcyaml
+            )
+
+            with modify_yaml(dvcyaml) as data:
+                artifacts = data.setdefault("artifacts", {})
+                artifacts.update({name: artifact.to_dict()})
+
+            self.repo.scm_context.track_file(dvcfile)
+
+        return artifacts.get(name)
diff --git a/dvc/schema.py b/dvc/schema.py
index 677f18292..30fea0d86 100644
--- a/dvc/schema.py
+++ b/dvc/schema.py
@@ -116,7 +116,6 @@ PLOT_DEFINITION = {
 SINGLE_PLOT_SCHEMA = {str: Any(PLOT_DEFINITION, None)}
 ARTIFACTS = "artifacts"
 SINGLE_ARTIFACT_SCHEMA = Schema({str: ARTIFACT_SCHEMA})
-ARTIFACTS_SCHEMA = Any(str, SINGLE_ARTIFACT_SCHEMA)
 FOREACH_IN = {
     Required(FOREACH_KWD): Any(dict, list, str),
     Required(DO_KWD): STAGE_DEFINITION,
@@ -130,7 +129,7 @@ MULTI_STAGE_SCHEMA = {
     VARS_KWD: VARS_SCHEMA,
     StageParams.PARAM_PARAMS: [str],
     StageParams.PARAM_METRICS: [str],
-    ARTIFACTS: ARTIFACTS_SCHEMA,
+    ARTIFACTS: SINGLE_ARTIFACT_SCHEMA,
 }
 
 COMPILED_SINGLE_STAGE_SCHEMA = Schema(SINGLE_STAGE_SCHEMA)


Test patch:
diff --git a/tests/func/artifacts/test_artifacts.py b/tests/func/artifacts/test_artifacts.py
index a632fdc9e..424d7f047 100644
--- a/tests/func/artifacts/test_artifacts.py
+++ b/tests/func/artifacts/test_artifacts.py
@@ -3,6 +3,7 @@ import os
 import pytest
 
 from dvc.annotations import Artifact
+from dvc.exceptions import InvalidArgumentError
 from dvc.repo.artifacts import name_is_compatible
 from dvc.utils.strictyaml import YAMLSyntaxError, YAMLValidationError
 
@@ -21,7 +22,7 @@ dvcyaml = {
 }
 
 
-def test_reading_artifacts_subdir(tmp_dir, dvc):
+def test_artifacts_read_subdir(tmp_dir, dvc):
     (tmp_dir / "dvc.yaml").dump(dvcyaml)
 
     subdir = tmp_dir / "subdir"
@@ -38,6 +39,55 @@ def test_reading_artifacts_subdir(tmp_dir, dvc):
     }
 
 
+def test_artifacts_add_subdir(tmp_dir, dvc):
+    subdir = tmp_dir / "subdir"
+    subdir.mkdir()
+
+    (subdir / "dvc.yaml").dump(dvcyaml)
+
+    new_art = Artifact(path="path")
+    tmp_dir.dvc.artifacts.add("new", new_art, dvcfile="subdir/dvc.yaml")
+
+    artifacts = {
+        name: Artifact(**values) for name, values in dvcyaml["artifacts"].items()
+    }
+    artifacts["new"] = new_art
+    assert tmp_dir.dvc.artifacts.read() == {
+        f"subdir{os.path.sep}dvc.yaml": artifacts,
+    }
+
+
+def test_artifacts_add_abspath(tmp_dir, dvc):
+    subdir = tmp_dir / "subdir"
+    subdir.mkdir()
+
+    new_art = Artifact(path="path")
+    tmp_dir.dvc.artifacts.add(
+        "new", new_art, dvcfile=os.path.abspath("subdir/dvc.yaml")
+    )
+
+    assert tmp_dir.dvc.artifacts.read() == {
+        f"subdir{os.path.sep}dvc.yaml": {"new": new_art},
+    }
+
+
+def test_artifacts_add_fails_on_dvc_subrepo(tmp_dir, dvc):
+    # adding artifact to the DVC subrepo from the parent DVC repo
+    # shouldn't work
+    subdir = tmp_dir / "subdir"
+    (subdir / ".dvc").mkdir(parents=True)
+
+    with pytest.raises(InvalidArgumentError):
+        tmp_dir.dvc.artifacts.add(
+            "failing", Artifact(path="path"), dvcfile="subdir/dvc.yaml"
+        )
+
+    with pytest.raises(InvalidArgumentError):
+        tmp_dir.dvc.artifacts.add(
+            "failing", Artifact(path="path"), dvcfile="subdir/dvclive/dvc.yaml"
+        )
+
+
 bad_dvcyaml_extra_field = {
     "artifacts": {
         "lol": {"kek": "cheburek", "path": "lol"},
@@ -71,7 +121,7 @@ artifacts:
 """
 
 
-def test_broken_dvcyaml_id_duplication(tmp_dir, dvc):
+def test_artifacts_read_fails_on_id_duplication(tmp_dir, dvc):
     with open(tmp_dir / "dvc.yaml", "w") as f:
         f.write(bad_dvcyaml_id_duplication)
 
@@ -79,32 +129,19 @@ def test_broken_dvcyaml_id_duplication(tmp_dir, dvc):
         tmp_dir.dvc.artifacts.read()
 
 
-dvcyaml_redirecting = {"artifacts": "artifacts.yaml"}
-
-
-def test_read_artifacts_yaml(tmp_dir, dvc):
-    (tmp_dir / "dvc.yaml").dump(dvcyaml_redirecting)
-    (tmp_dir / "artifacts.yaml").dump(dvcyaml["artifacts"])
-
-    artifacts = {
-        name: Artifact(**values) for name, values in dvcyaml["artifacts"].items()
-    }
-    assert tmp_dir.dvc.artifacts.read() == {
-        "dvc.yaml": artifacts,
-    }
-
-
 @pytest.mark.parametrize(
     "name",
     [
+        "1",
         "m",
         "nn",
         "m1",
+        "1nn",
         "model-prod",
         "model-prod-v1",
     ],
 )
-def test_check_name_is_valid(name):
+def test_name_is_compatible(name):
     assert name_is_compatible(name)
 
 
@@ -112,10 +149,8 @@ def test_check_name_is_valid(name):
     "name",
     [
         "",
-        "1",
         "m/",
         "/m",
-        "1nn",
         "###",
         "@@@",
         "a model",
@@ -125,8 +160,7 @@ def test_check_name_is_valid(name):
         "model@1",
         "model#1",
         "@namespace/model",
-        "namespace/model",
     ],
 )
-def test_check_name_is_invalid(name):
+def test_name_is_compatible_fails(name):
     assert not name_is_compatible(name)