exp: ignore workspace errors during push/pull#10128
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10128 +/- ##
==========================================
- Coverage 90.66% 90.40% -0.27%
==========================================
Files 488 494 +6
Lines 37425 37649 +224
Branches 5442 5481 +39
==========================================
+ Hits 33932 34037 +105
- Misses 2857 2951 +94
- Partials 636 661 +25 ☔ View full report in Codecov by Sentry. |
| if rev == "workspace" and rev not in revs: | ||
| continue | ||
| if onerror: | ||
| onerror(rev, None, exc) | ||
| collection_exc = exc |
There was a problem hiding this comment.
Looking at the code, it seems we never raise error, unless there are no indexes.
So, it seems dvc exp push will only fail if both of the workspace and experiment ref failed to get collected, or if we have no experiment to push, but only the workspace that has failed to get collected.
There was a problem hiding this comment.
If there is an error only in the workspace, it will not fail, but it will loudly log an error message that looks like something went wrong. For example:
$ dvc exp push origin
ERROR: failed to collect 'workspace' - failed to parse 'stages.params_test.cmd' in 'dvc.yaml': Could not find 'params'
Pushed experiment rival-knap to Git remote 'origin'.
1 file uploaded
See also the test below.
There was a problem hiding this comment.
That means we should not use logging.exception in this case (which should only be used before raising an exception), and the error message should just suggest that it was skipped due to the failure.
There was a problem hiding this comment.
That means we should not use logging.exception in this case
Okay, I pushed another commit where I changed the logging to a warning. However, I am still skipping it for the workspace in the case where other revs were passed that don't include the workspace. It seems misleading to show warnings about the workspace rev in these cases since the workspace is irrelevant (for example, when pushing or pulling experiments).
There was a problem hiding this comment.
TBH I think the "right" solution here is not to fetch the workspace at all. It's wasteful in this case, but I was trying to limit the changes.
There was a problem hiding this comment.
I looked where we are calling fetch, and I am fine with this PR, but I have to note we are changing behaviour here (i.e on --all-commits we are skipping workspace).
There was a problem hiding this comment.
It also uses this method.
There was a problem hiding this comment.
Right, I see that's also documented in https://dvc.org/doc/command-reference/push#-A, so I'll limit to dvc exp push/pull for now.
There was a problem hiding this comment.
It also uses this method.
My apologies, _collect_indexes in fetch keeps on confusing me. Doesn't make any sense.
Especially with a small GitHhub diffview. 😅
There was a problem hiding this comment.
Okay, limited it to exp push/pull
|
Code changes look good to me. cc'ing @efiop to review once. |
3 participants
Fixes #9768. Even for operations that don't use the workspace, like
dvc exp push, dvc was logging errors if the workspace state was invalid, making it look like the command failed. This PR suppresses those errors if the workspace was not one of the included revs.