Skip to content

render: avoid deepcopying #9033

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

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Conversation

skshetry
Copy link
Collaborator

@skshetry skshetry commented Feb 15, 2023

A low hanging fruit that reduces 20% of runtime when run in vscode-dvc-demo with:

dvc plots diff $(git log --oneline --pretty=format:"%h")

Screenshot from 2023-02-15 13-11-43

@skshetry skshetry added the optimize Optimizes DVC label Feb 15, 2023
@skshetry skshetry requested a review from daavoo February 15, 2023 07:30
@skshetry skshetry added the A: plots Related to the plots label Feb 15, 2023
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 93.07% // Head: 93.06% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (c417478) compared to base (6f87c14).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9033      +/-   ##
==========================================
- Coverage   93.07%   93.06%   -0.02%     
==========================================
  Files         456      456              
  Lines       36814    36758      -56     
  Branches     5335     5324      -11     
==========================================
- Hits        34264    34207      -57     
- Misses       2025     2026       +1     
  Partials      525      525              
Impacted Files Coverage Δ
dvc/render/convert.py 87.09% <100.00%> (-0.41%) ⬇️
dvc/render/converter/vega.py 91.20% <100.00%> (-0.05%) ⬇️
dvc/repo/diff.py 95.31% <0.00%> (-3.00%) ⬇️
dvc/output.py 89.18% <0.00%> (-0.30%) ⬇️
tests/func/test_diff.py 100.00% <0.00%> (ø)
dvc/repo/index.py 89.88% <0.00%> (+0.57%) ⬆️

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.

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skshetry
Copy link
Collaborator Author

This is clearly another low-hanging fruit:

iterative/dvc-render@2aa47c8/src/dvc_render/vega_templates.py#L66-L71

Goes back to iterative/dvc-render#23 / iterative/dvc-render#28 (not treating the template content as string)

Thanks for the links. Seems a bit involved though, right? Does that keep backward-compatibility?

@skshetry skshetry merged commit be824ed into iterative:main Feb 15, 2023
@skshetry skshetry deleted the avoid-deepcopying branch February 15, 2023 10:40
@daavoo
Copy link
Contributor

daavoo commented Feb 16, 2023

This is clearly another low-hanging fruit:
iterative/dvc-render@2aa47c8/src/dvc_render/vega_templates.py#L66-L71
Goes back to iterative/dvc-render#23 / iterative/dvc-render#28 (not treating the template content as string)

Thanks for the links. Seems a bit involved though, right?

I can give it a quick try as I have recently looked at it.

Does that keep backward-compatibility?

It should as there are internal operations, I/O should remain the same

@skshetry
Copy link
Collaborator Author

skshetry commented Feb 16, 2023

Thank you. FYI I ran the above benchmark with dvc pull --all-commits, so the collect() is not very pronounced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Related to the plots optimize Optimizes DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants