exp run: add experiment name check. #6848

karajan1001 · 2021-10-21T09:37:20Z

Add experiment name check (https://git-scm.com/docs/git-check-ref-format)
Add duplicate exp name check.
Add some unit test for it.

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@pmrowla , in #6819, exp name with '/' inside are request. And these types of exp names are stored in a subdirectory. For example in #6843, it is stored in .git/refs/heads/dependabot/pip/pylint-2.11.1. The subdirectory here is used for grouping branches. Currently, our dvc exp did not support this. It involves not only name checking here, maybe we should reopen #6819 and create a new PR for it?

pmrowla · 2021-10-21T09:51:29Z

@pmrowla , in #6819, exp name with '/' inside are request. And these types of exp names are stored in a subdirectory. For example in #6843, it is stored in .git/refs/heads/dependabot/pip/pylint-2.11.1. The subdirectory here is used for grouping branches. Currently, our dvc exp did not support this. It involves not only name checking here, maybe we should reopen #6819 and create a new PR for it?

We should not support slashes in experiment names. Yes, the full ref can contain slashes, and we already use slashes in exp refs to organize them, but the user-readable (short) name should not contain slashes - the same as git branch or tag (short) names.

karajan1001 · 2021-10-21T09:59:28Z

We should not support slashes in experiment names. Yes, the full ref can contain slashes, and we already use slashes in exp refs to organize them, but the user-readable (short) name should not contain slashes - the same as git branch or tag (short) names.

Git branches can have slashes inside them, in the case the slashes will be regarded as directory name splits. And the branches will be well organized in groups.

pmrowla · 2021-10-21T10:25:15Z

Ah ok, that's my misunderstanding then, I didn't think cgit allowed you to nest anything in refs/heads. I guess that could be re-opened as a feature request, but I'm still not sure we should support it, unless it becomes something that is more frequently requested from users. I'd prefer to just keep everything in refs/exps/... namespaces completely reserved for DVC use for now.

karajan1001 · 2021-10-21T10:29:26Z

Ah ok, that's my misunderstanding then, I didn't think cgit allowed you to nest anything in refs/heads. I guess that could be re-opened as a feature request, but I'm still not sure we should support it, unless it becomes something that is more frequently requested from users. I'd prefer to just keep everything in refs/exps/... namespaces completely reserved for DVC use for now.

I will ban it in this PR, and then clarify in doc.

dvc/command/experiments.py

tests/unit/command/test_experiments.py

dberenbaum · 2021-10-22T12:11:01Z

I'd prefer to just keep everything in refs/exps/... namespaces completely reserved for DVC use for now.

Is it harder to handle nested refs in refs/exps? Or is there some other reason not to allow slashes? @karajan1001 makes a good point that this could be a helpful way to organize, plus it would solve #6819.

1. Add experiment name check (https://git-scm.com/docs/git-check-ref-format) 2. Add duplicate exp name check. 3. Add some unit test for it.

karajan1001 · 2021-10-24T02:43:59Z

I'd prefer to just keep everything in refs/exps/... namespaces completely reserved for DVC use for now.

Is it harder to handle nested refs in refs/exps? Or is there some other reason not to allow slashes? @karajan1001 makes a good point that this could be a helpful way to organize, plus it would solve #6819.

The dulwich backend support / in exp name, and actually our full reference is inside the subdirectory named by the HEAD revision. But still some problems on the DVC side, for example currently we use / to separate refname and the last part as the human-readable one, this should be changed. I don't think it is very difficult to support / in ref name, but still need some effort.

pmrowla · 2021-10-25T03:12:40Z

I'd prefer to just keep everything in refs/exps/... namespaces completely reserved for DVC use for now.

Is it harder to handle nested refs in refs/exps? Or is there some other reason not to allow slashes? @karajan1001 makes a good point that this could be a helpful way to organize, plus it would solve #6819.

There's nothing preventing us from allowing slashes in the exp refs, but once we start allowing them it means we cannot extend the current naming convention of refs/exps/ab/cd1234/.../<exp_name> to include any new information in the ... portion of the ref. My main concern is whether or not we think the current state of exp/refs is stable enough to make this change. I'd prefer not to make the change right now, unless using slashes in names really is a hard requirement.

Regarding user naming conventions, dots . and dashes - are allowed in exp names, and can also be used to generate hierarchies which could be filtered/sorted by users in the same way as using slash.

dvc/repo/experiments/__init__.py

tests/unit/repo/experiments/test_utils.py

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

pmrowla · 2021-10-25T06:25:18Z

dvc/repo/experiments/__init__.py

+        name = kwargs.pop("name", None)
+        self._validate_ref_name(name, **kwargs)
+
+        return self._stash_exp(*args, name=name, **kwargs)


It looks a bit strange to pop a kwarg and then re-use it again in the following call (to _stash_exp

Suggested change

name = kwargs.pop("name", None)

self._validate_ref_name(name, **kwargs)

return self._stash_exp(*args, name=name, **kwargs)

self._validate_ref_name(kwargs.get("name"), **kwargs)

return self._stash_exp(*args, **kwargs)

Alternatively, you could also just make name default to None in _validate_ref_name and call it with **kwargs

The problem is that kwargs still contains name argument and this would cause a duplication. If we want to use this method, we need to get name, force, reset and pass them separately.

Seems like the simplest fix is to just do

def _validate_ref_name(self, name: Optional[str] = None, **kwargs):

and call it from here as

self._validate_ref_name(**kwargs)

You could also change this function to be

def new(self, *args, checkpoint_resume: Optional[str] = None, name: Optional[str] = None, **kwargs):

and then keep the call you have as

self._validate_ref_name(name, **kwargs) return self._stash_exp(*args, name=name, **kwargs)

The point is just that doing

value = kwargs.pop(key) func(key=value, **kwargs)

isn't particularly clean code

Seems like the simplest fix is to just do

def _validate_ref_name(self, name: Optional[str] = None, **kwargs):
and call it from here as

self._validate_ref_name(**kwargs)

Yes, I also considered it, but I think it is more clear to call it explicitly.

dvc/repo/experiments/__init__.py

karajan1001 · 2021-10-25T10:06:44Z

dvc/repo/experiments/__init__.py

            "\tdvc exp branch <exp> <branch>\n"
        )

+    def _validate_new_ref(self, exp_ref: ExpRefInfo):


This function is used for validating a new reference, not only the name. Because we include duplication detect in it. And also the duplication detection is not to the name but to the whole reference.

dberenbaum · 2021-10-25T14:41:38Z

I'd prefer not to make the change right now, unless using slashes in names really is a hard requirement.

Sounds good. I don't think there's a hard requirement for now at least, but helpful to have more context about the rationale.

dberenbaum · 2021-10-25T17:12:45Z

Looks good! One small request: can we remove "Reproduced" from https://github.com/iterative/dvc/blob/c07592131a616afc51f093ca4d4fe8615b63062e/dvc/repo/experiments/base.py#L39?

jorgeorpinel · 2021-12-02T19:27:48Z

From what I understand this fixes bugs so no docs changes are needed (I checked the box).

karajan1001 · 2021-12-03T07:48:44Z

From what I understand this fixes bugs so no docs changes are needed (I checked the box).

This PR forbids the duplicate name and name with some illegal character (space, asterisk, etc), in my opinion, better to make an error msg if someone wants to use an illegal name.

rel. treeverse/dvc#6848

jorgeorpinel · 2021-12-07T01:20:28Z

OK I created treeverse/dvc.org#3065 then.

* ref: state exp names shoold be unique rel. treeverse/dvc#6848 * Restyled by prettier (#3066) Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

karajan1001 requested a review from a team as a code owner October 21, 2021 09:37

karajan1001 requested a review from pmrowla October 21, 2021 09:37

karajan1001 mentioned this pull request Oct 21, 2021

exp run: Parameter name does not support backslashes #6819

Closed

pmrowla assigned karajan1001 Oct 22, 2021

pmrowla added A: experiments Related to dvc exp ui user interface / interaction labels Oct 22, 2021

pmrowla suggested changes Oct 22, 2021

View reviewed changes

dvc/command/experiments.py Outdated Show resolved Hide resolved

pmrowla reviewed Oct 22, 2021

View reviewed changes

dvc/command/experiments.py Outdated Show resolved Hide resolved

pmrowla reviewed Oct 22, 2021

View reviewed changes

tests/unit/command/test_experiments.py Outdated Show resolved Hide resolved

karajan1001 added 3 commits October 24, 2021 10:36

Add experiment name check.

1b9b4cd

1. Add experiment name check (https://git-scm.com/docs/git-check-ref-format) 2. Add duplicate exp name check. 3. Add some unit test for it.

Ban slash / in dvc exp names

8c944b6

Use dulwich backend for ref name checking

b812d61

karajan1001 force-pushed the exp_name_check branch from e75b4eb to b812d61 Compare October 24, 2021 02:36

Some bug fix

715d199

karajan1001 changed the title ~~Add experiment name check.~~ exp run: add experiment name check. Oct 25, 2021

pmrowla suggested changes Oct 25, 2021

View reviewed changes

dvc/repo/experiments/__init__.py Outdated Show resolved Hide resolved

dvc/repo/experiments/__init__.py Outdated Show resolved Hide resolved

tests/unit/repo/experiments/test_utils.py Outdated Show resolved Hide resolved

karajan1001 and others added 2 commits October 25, 2021 11:38

Update dvc/repo/experiments/__init__.py

0125083

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

Update dvc/repo/experiments/__init__.py

9038fda

Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>

karajan1001 mentioned this pull request Oct 25, 2021

[WIP] exp run: add support for backslashes in ref names #6861

Closed

2 tasks

Some review changes

2120cde

pmrowla reviewed Oct 25, 2021

View reviewed changes

dvc/repo/experiments/__init__.py Outdated Show resolved Hide resolved

Make some funtion more reusable.

3f79e7b

karajan1001 commented Oct 25, 2021

View reviewed changes

pmrowla approved these changes Oct 25, 2021

View reviewed changes

pmrowla merged commit c075921 into treeverse:master Oct 25, 2021

karajan1001 deleted the exp_name_check branch October 25, 2021 12:01

dberenbaum mentioned this pull request Oct 25, 2021

fix msg in for ExperimentExistsError #6864

Merged

jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Dec 7, 2021

ref: state exp names shoold be unique

f388a53

rel. treeverse/dvc#6848

jorgeorpinel mentioned this pull request Dec 7, 2021

ref: state exp names should be unique treeverse/dvc.org#3065

Merged

dberenbaum mentioned this pull request Jan 27, 2022

exp show --csv: Parent column empty #7001

Closed

pmrowla mentioned this pull request Jun 26, 2022

exp show: branch name containing path separator not returned for HEAD #7933

Closed

exp run: add experiment name check. #6848

exp run: add experiment name check. #6848

Uh oh!

Conversation

karajan1001 commented Oct 21, 2021 • edited by jorgeorpinel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmrowla commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karajan1001 commented Oct 21, 2021

Uh oh!

pmrowla commented Oct 21, 2021

Uh oh!

karajan1001 commented Oct 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dberenbaum commented Oct 22, 2021

Uh oh!

karajan1001 commented Oct 24, 2021

Uh oh!

pmrowla commented Oct 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmrowla Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

karajan1001 Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

pmrowla Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

pmrowla Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karajan1001 Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Oct 25, 2021

Uh oh!

dberenbaum commented Oct 25, 2021

Uh oh!

jorgeorpinel commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karajan1001 commented Dec 3, 2021

Uh oh!

jorgeorpinel commented Dec 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karajan1001 commented Oct 21, 2021 •

edited by jorgeorpinel

Loading

pmrowla commented Oct 21, 2021 •

edited

Loading

pmrowla Oct 25, 2021 •

edited

Loading

karajan1001 Oct 25, 2021 •

edited

Loading

jorgeorpinel commented Dec 2, 2021 •

edited

Loading