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 save: simplify implementation #8690

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Dec 13, 2022

Gets rid of the queue/executor mechanism to save experiments.

fixes #8680

@dtrifiro dtrifiro requested a review from daavoo December 13, 2022 20:16
@dtrifiro dtrifiro enabled auto-merge (rebase) December 13, 2022 20:16
@dtrifiro dtrifiro marked this pull request as draft December 13, 2022 20:21
auto-merge was automatically disabled December 13, 2022 20:21

Pull request was converted to draft

@dtrifiro
Copy link
Contributor Author

Gonna add a few more tests

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Base: 93.52% // Head: 93.51% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (8fd6ffb) compared to base (6fc0a6f).
Patch coverage: 98.24% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8690      +/-   ##
==========================================
- Coverage   93.52%   93.51%   -0.02%     
==========================================
  Files         457      457              
  Lines       36128    36121       -7     
  Branches     5224     5229       +5     
==========================================
- Hits        33790    33779      -11     
- Misses       1837     1839       +2     
- Partials      501      503       +2     
Impacted Files Coverage Δ
tests/unit/repo/experiments/test_utils.py 100.00% <ø> (ø)
dvc/repo/experiments/save.py 94.87% <96.77%> (-5.13%) ⬇️
dvc/repo/experiments/exceptions.py 81.03% <100.00%> (ø)
dvc/repo/experiments/executor/local.py 88.54% <100.00%> (-2.25%) ⬇️
tests/func/experiments/test_save.py 100.00% <100.00%> (ø)
dvc/repo/commit.py 81.81% <0.00%> (-6.07%) ⬇️

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.

@dtrifiro dtrifiro force-pushed the simplify-exp-save branch 2 times, most recently from fb31e6a to ce5668f Compare December 15, 2022 09:49
@dtrifiro dtrifiro marked this pull request as ready for review December 15, 2022 09:49
@dtrifiro dtrifiro force-pushed the simplify-exp-save branch 2 times, most recently from c0364ea to f07741f Compare December 20, 2022 09:40
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.

Probably would be good to update the force description in the cmd (

help="Save even if hash value for dependencies/outputs changed.",
):

@daavoo daavoo linked an issue Dec 20, 2022 that may be closed by this pull request
@jorgeorpinel
Copy link
Contributor

Hi. Are you planning to update docs after this? Not sure if actual behavior will be affected but if so, feel free to take over iterative/dvc.org#4200 to include them there.

@dtrifiro
Copy link
Contributor Author

dtrifiro commented Dec 22, 2022

Hey @jorgeorpinel, behaviour should be identical to the current implementation, so there's no need to update the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exp branch: includes extra files when used with exp save
4 participants