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

metrics: Skip revisions with corrupt dvc.lock files #5845

Closed
wants to merge 1 commit into from

Conversation

jheister
Copy link

Fixes #5844

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@pmrowla
Copy link
Contributor

pmrowla commented Apr 20, 2021

Hi @jheister, thanks for the PR. This does correct the specific metrics show issue you found in #5844, but as @dberenbaum noted already, this is really related to a larger problem in DVC (#5525).

There are more types of potential errors we need to be looking for here than just LockfileCorruptedError, and ideally the fix for this will go in repo.collect and possibly repo.brancher since the underlying problem is not actually specific to metrics - it's really related to how we collect pipelines/stages across git commits in general.

I'll leave this PR open for now, but I'm not sure it will get merged since we are already planning on addressing the problem with a more general fix for #5525.

@pared pared self-requested a review April 20, 2021 14:06
@pared pared self-assigned this Apr 20, 2021
@pared
Copy link
Contributor

pared commented Apr 21, 2021

@jheister
Thank your for your contribution!
As @pmrowla mentioned we are currently working on two issues originating in same problem: #5667 and #5525. While your solution is correct, it solves only one of multiple problems that can occur during collection of metrics. To get rid of the source problem we need to fix this problem for all possible Exceptions thrown (during collection) and for all commands related to collecting metrics/params/plots from the repository. So the fix here needs to cover dvc plots/params/metrics/exp show commands. As I mentioned we are working on that, and probably the fix will cover also this use case. We cannot merge this fix, until we have all the use cases covered (and once we have them this fix might be too specific to be included). Having said so I would like to keep this PR open until we propose the "general" solution, to be sure we do not omit this particular use case.

@efiop
Copy link
Member

efiop commented Jul 18, 2021

Closing in favor of #5525

@efiop efiop closed this Jul 18, 2021
@pared
Copy link
Contributor

pared commented Jul 19, 2021

@jheister seems like #5984 fixed the issue

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.

metrics show --all-commits fails on corrupt historic commits
4 participants