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

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

Closed
mattseddon opened this issue Jun 26, 2022 · 8 comments · Fixed by #8425
Closed

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

mattseddon opened this issue Jun 26, 2022 · 8 comments · Fixed by #8425
Assignees
Labels
A: experiments Related to dvc exp product: VSCode Integration with VSCode extension

Comments

@mattseddon
Copy link
Member

mattseddon commented Jun 26, 2022

Bug Report

From iterative/vscode-dvc#1946

Description

exp show will not return the full name of a branch for the HEAD revision if it contains a path separator.

This causes two issues in the extension:

  1. The wrong name is shown in our experiments table.
  2. We call plots diff with the incomplete branch name and it fails (we use the name to fetch the HEAD revision).

e.g when the branch name is readme/known-limitations the output is as follows:

/vscode-dvc/demo readme/known-limitations *1 !2 ❯ dvc exp show --show-json
{
  "workspace": {
  ...
  },
  "4d78b9e7158ee1d6504028f79f65daf48d2d9af3": {
    "baseline": {
      "data": {
       ...
        "name": "known-limitations"
      }
    }
  }
}

Reproduce

  1. checkout a branch with a name which includes a file path separator (e.g readme/known-limitations)
  2. run exp show
  3. the last part of the branch name will be returned as the name for the HEAD commit (e.g known-limitations).

Expected

exp show returns the full branch name including file separators.

Environment information

Output of dvc doctor:

$ dvc doctor
DVC version: 2.11.0 (brew)
---------------------------------
Platform: Python 3.9.13 on macOS-12.4-x86_64-i386-64bit
Supports:
        azure (adlfs = 2022.4.0, knack = 0.9.0, azure-identity = 1.10.0),
        gdrive (pydrive2 = 1.10.1),
        gs (gcsfs = 2022.5.0),
        webhdfs (fsspec = 2022.5.0),
        http (aiohttp = 3.8.1, aiohttp-retry = 2.4.6),
        https (aiohttp = 3.8.1, aiohttp-retry = 2.4.6),
        s3 (s3fs = 2022.5.0, boto3 = 1.21.21),
        ssh (sshfs = 2022.6.0),
        oss (ossfs = 2021.8.0),
        webdav (webdav4 = 0.9.7),
        webdavs (webdav4 = 0.9.7)
Cache types: reflink, hardlink, symlink
Cache directory: apfs on /dev/disk1s5s1
Caches: local
Remotes: s3
Workspace directory: apfs on /dev/disk1s5s1
Repo: dvc (subdir), git

Additional Information (if any):

Please LMK if this behaviour is by design. Thank you.

Original issue: iterative/vscode-dvc#1946

I have tested to see if calling dvc plots diff nested/branch works and it fails if there are any static plots involved. I will raise a separate issue for this.

@mattseddon mattseddon added product: VSCode Integration with VSCode extension A: experiments Related to dvc exp bug labels Jun 26, 2022
@mattseddon mattseddon changed the title exp show: nested branch name not returned for HEAD exp show: branch name containing path separator not returned for HEAD Jun 26, 2022
@pmrowla
Copy link
Contributor

pmrowla commented Jun 26, 2022

For experiments this is expected and intentional, exp names are not allowed to contain slashes (even though they are valid in git refs)

see: #6848 (comment)

e: reopened since for branches/tags we should still display them with the slash

@pmrowla pmrowla closed this as completed Jun 26, 2022
@pmrowla pmrowla reopened this Jun 26, 2022
@shcheklein
Copy link
Member

@mattseddon can we do a workaround for now ... pass hash id instead into dvc plots diff?

@mattseddon
Copy link
Member Author

I have been looking at how to make that hack. I would need to create a mapping between the name and the sha but the fix is not as simple as you would initially think. The returned plots data will have the hash id all through it as the rev field. We will have to replace all instances of that hash id in the data in order to show the semi-correct name in the UI/show the correct plots data.

@shcheklein
Copy link
Member

The returned plots data will have the hash id all through it as the rev field

Yes, and it might be fine for now as a workaround for names that we know are problematic, wdyt? Image plots will be fine, trends also (?), only plots like confusion matrixes will have these hash ids?

@dberenbaum
Copy link
Collaborator

e: reopened since for branches/tags we should still display them with the slash

What's the level of effort to support it?

For experiments this is expected and intentional, exp names are not allowed to contain slashes (even though they are valid in git refs)

see: #6848 (comment)

The comment suggests hesitance to support slashes because the exp refs conventions aren't stable enough. Do you still have those concerns @pmrowla?

@pmrowla
Copy link
Contributor

pmrowla commented Jun 30, 2022

This is just a UI issue, it is separate from the "should exp names be allowed to contain slashes" question. The problem here is that if you have a git branch named foo/bar, we display it as bar due to something parsing git branches by splitting at the last path separator.

Exp names should still be prohibited from having slashes. But git branches and tags are allowed to have slashes, and we need to display them properly in DVC commands

@shcheklein
Copy link
Member

If it's a small effort, could we please address this?

@dberenbaum dberenbaum added this to DVC Oct 10, 2022
@dberenbaum dberenbaum moved this to Backlog in DVC Oct 10, 2022
@daavoo
Copy link
Contributor

daavoo commented Oct 11, 2022

If it's a small effort, could we please address this?

I will open P.R. later

@daavoo daavoo self-assigned this Oct 11, 2022
daavoo added a commit that referenced this issue Oct 11, 2022
@daavoo daavoo linked a pull request Oct 11, 2022 that will close this issue
daavoo added a commit that referenced this issue Oct 11, 2022
pmrowla pushed a commit that referenced this issue Oct 11, 2022
Repository owner moved this from Backlog to Done in DVC Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Related to dvc exp product: VSCode Integration with VSCode extension
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants