Skip to content

Conversation

@karajan1001
Copy link
Contributor

fix: #144

In our current describe function we only accept one rev at a time and will try to get a reference for every reference in the repo. It will cost O(MN) in the exp show in which we try to get a ref for every single experiment.

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Base: 82.25% // Head: 82.23% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (8b905d7) compared to base (ed17985).
Patch coverage: 96.15% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
- Coverage   82.25%   82.23%   -0.02%     
==========================================
  Files          25       25              
  Lines        3353     3367      +14     
  Branches      574      578       +4     
==========================================
+ Hits         2758     2769      +11     
- Misses        515      517       +2     
- Partials       80       81       +1     
Impacted Files Coverage Δ
src/scmrepo/git/backend/base.py 80.59% <ø> (ø)
src/scmrepo/git/backend/gitpython.py 64.42% <ø> (ø)
src/scmrepo/git/backend/pygit2.py 79.38% <ø> (ø)
src/scmrepo/git/backend/dulwich/__init__.py 81.41% <87.50%> (-0.02%) ⬇️
src/scmrepo/git/__init__.py 84.05% <100.00%> (+0.54%) ⬆️
tests/test_git.py 100.00% <100.00%> (ø)
tests/conftest.py 93.00% <0.00%> (-2.00%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@skshetry
Copy link
Collaborator

@karajan1001, can you please post the new results of the profiling with this patch in the same way that you did on #144?

Also before/after runtime for a real dvc exp show would be great.

@karajan1001
Copy link
Contributor Author

@karajan1001, can you please post the new results of the profiling with this patch in the same way that you did on #144?

Also before/after runtime for a real dvc exp show would be great.

In iterative/dvc#8453. In the same repo, this version of describe runs 5 times faster than the previous one. (With the experiment) grows the difference will be even bigger.

@karajan1001 karajan1001 requested a review from pmrowla October 19, 2022 10:30
Copy link
Collaborator

@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.

I don't think I have enough understanding about this on the exp side. So I'll leave this up to @pmrowla.

I will suggest considering RefsIterator or something similar concept that allows us to do this kind of thing, as it seems this way we may need to extend multiple APIs to support lists, breaking the API. Also repo.refs of dulwich comes to mind, but it's limited (but we can extend :) ).

Anyway, I'm okay with this for the time being. :)

@pmrowla pmrowla merged commit 74b3a98 into iterative:main Oct 20, 2022
@karajan1001 karajan1001 deleted the fix144 branch October 20, 2022 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add multi rev support for function describe

4 participants