-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fix plots not showing on first experiment run #4412
Conversation
|
||
plotPath.revisions = new Set([...existing.revisions, ...revisions]) | ||
|
||
if (!plotPath.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how common this bug is but when adding the plots to dvc.yaml in example-get-started and running an experiment, plot diff
will first send plots with file not found
errors. This causes us to build our plot paths without types that never get added since the code only updates revisions if the path already exists.
I updated it to check the existing path's type existence but there might be a better way to handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the !plotPath.type
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a regression test
I'll review today. |
Code Climate has analyzed commit 7bb5e0d and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
} | ||
: existing | ||
) | ||
return updateExistingPlotPath(acc, data, hasChildren, revisions, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[I] In updateExistingPlotPath
you are looping over the entire array again. This could be cut down to a single loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*I realise that this change is just expanding on what the old code was doing but we can still improve/update.
main
I shorted the demos since the experiment run takes over a minute to run on my computer at the moment 😅
trimmed.2.mov
PR
trimmed.1.mov
Fixes bug mentioned in slack thread