exp show: Use batch call on scm.describe
#8453
Conversation
karajan1001
added
performance
karajan1001
changed the title
scm.describescm.describe
dvc/repo/experiments/show.py
Outdated
| rev_tag = repo.scm.describe(rev_set, base="refs/tags") | ||
| rev_head = repo.scm.describe(rev_set, base="refs/heads") | ||
| exact_name = repo.experiments.get_exact_name(rev_set) |
There was a problem hiding this comment.
rev_set should remove matches after each call (once we hit a tag for a revision, we no longer need to check branches, and so on) so that we can skip additional describe/get_exact_name calls if we already have matches for all of our revs
so maybe something like
names = {}
for base in ("refs/tags/", "refs/heads/"):
if rev_set:
names.update(
(rev, ref[len(base):])
for rev, ref in scm.describe(rev_set, base=base)).items()
if ref is not None
)
rev_set.difference_update(names.keys())
# update names for exps
...
dvc/repo/experiments/show.py
Outdated
| if rev == "workspace": | ||
| continue | ||
| if rev_tag[rev]: | ||
| name = rev_tag[rev].replace("refs/tags/", "") |
There was a problem hiding this comment.
This is addressed in the other suggested change, but this should really be
name = rev_tag[rev][10:] # 10 == len('refs/tags/')
replace() is slower and it would also remove ocurrences later in the tag. (technically you could have a ref that looks like refs/tags/my-tag-refs/tags where the tag is named my-tag-refs/tags)
dvc/commands/experiments/ls.py
Outdated
| branch = self.repo.scm.describe(baseline, base="refs/heads") | ||
| branch = branches[tag] | ||
| if branch: | ||
| tag = branch.split("/")[-1] |
There was a problem hiding this comment.
This split should be removed (same bug as #7933)
There was a problem hiding this comment.
It will make the output like:
refs/heads/main:
exp-f88ff
There was a problem hiding this comment.
It will make the output like:
refs/heads/main: exp-f88ff
We should only keep / for names from tags or branches.
There was a problem hiding this comment.
When you call describe to get the name you have to remove the refs/.../ prefix (using len(base)) and not rely on split.
There was a problem hiding this comment.
The output should be
main:
exp-f88ff
pmrowla
left a comment
pmrowla
left a comment
There was a problem hiding this comment.
LGTM other than including the additional split bugfix
fix: iterative#8451 1. Change the call of scm.describe from individual revision to a collection of them. Accelerate the run speed of `exp show` 2. Bump scmrepo to 0.1.2
2 participants
wait for iterative/scmrepo#145 to merge first.
fix: #8451
nearly five times faster in my local workspace for an
exp show, no primary time costing method inexp showfor now.exp show❗ 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. 🙏