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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Fix plots not showing on first experiment run #4412

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
205e15b
Fix plots not showing on first experiment run
julieg18 Aug 3, 2023
7bb5e0d
Merge branch 'main' into fix-plots-not-showing-on-first-exp-run
mattseddon Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 44 additions & 25 deletions extension/src/plots/paths/collect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,10 @@ const collectType = (plots: Plot[]) => {
? type.add(PathType.TEMPLATE_MULTI)
: type.add(PathType.TEMPLATE_SINGLE)
}

return type
}

const getType = (
data: PlotsData,
hasChildren: boolean,
path: string
): Set<PathType> | undefined => {
if (hasChildren) {
return
}

const getType = (data: PlotsData, path: string): Set<PathType> | undefined => {
const plots = data[path]
if (!definedAndNonEmpty(plots)) {
return
Expand Down Expand Up @@ -118,6 +109,46 @@ const collectPathRevisions = (data: PlotsData, path: string): Set<string> => {
return revisions
}

const collectPlotPathType = (
plotPath: PlotPath,
data: PlotsData,
hasChildren: boolean,
path: string
) => {
if (hasChildren) {
return
}

const type = getType(data, path)

if (type) {
plotPath.type = type
}
}

const updateExistingPlotPath = (
acc: PlotPath[],
data: PlotsData,
hasChildren: boolean,
revisions: Set<string>,
path: string
) =>
acc.map(existing => {
const plotPath = { ...existing }

if (existing.path !== path) {
return plotPath
}

plotPath.revisions = new Set([...existing.revisions, ...revisions])

if (!plotPath.type) {
Copy link
Contributor Author

@julieg18 julieg18 Aug 3, 2023 •
edited
Loading

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.

Copy link
Member

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.

collectPlotPathType(plotPath, data, hasChildren, path)
}

return plotPath
})

const collectOrderedPath = (
acc: PlotPath[],
data: PlotsData,
Expand All @@ -126,31 +157,20 @@ const collectOrderedPath = (
idx: number
): PlotPath[] => {
const path = getPath(pathArray, idx)
const hasChildren = idx !== pathArray.length

if (acc.some(({ path: existingPath }) => existingPath === path)) {
return acc.map(existing =>
existing.path === path
? {
...existing,
revisions: new Set([...existing.revisions, ...revisions])
}
: existing
)
return updateExistingPlotPath(acc, data, hasChildren, revisions, path)
Copy link
Member

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

Copy link
Member

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.

}

const hasChildren = idx !== pathArray.length

const plotPath: PlotPath = {
hasChildren,
parentPath: getParent(pathArray, idx),
path,
revisions
}

const type = getType(data, hasChildren, path)
if (type) {
plotPath.type = type
}
collectPlotPathType(plotPath, data, hasChildren, path)

acc.push(plotPath)
return acc
Expand Down Expand Up @@ -228,7 +248,6 @@ export const collectPaths = (
if (errors?.length) {
acc = collectErrorPaths(acc, data, errors)
}

return acc
}

Expand Down