Skip to content

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
Contributor

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