Add deps to experiment table and columns tree #1830
Conversation
mattseddon
force-pushed
the
add-deps-to-experiment-table
branch
from
1b4fa68 to
2483338
Compare
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| ) | ||
| } | ||
|
|
||
| const walkValueTree = ( |
There was a problem hiding this comment.
Function walkValueTree has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
mattseddon
force-pushed
the
add-deps-to-experiment-table
branch
from
00190fe to
a7b0dd3
Compare
| "description": "Color to indicate a DVC dep column in the experiments table", | ||
| "defaults": { | ||
| "dark": "#b079e7", | ||
| "light": "#4f1886" |
There was a problem hiding this comment.
[C] I'm open to suggestions on these.
| group: type, | ||
| id: path, | ||
| name | ||
| name: label |
There was a problem hiding this comment.
[F] Although we need name for react-table we now need label for both the plots and columns trees. It makes sense to use label throughout the extension and name: label here for consistency.
| const paramWithNumbers = param.find(p => p.label === 'withNumbers') as Column | ||
| const paramWithoutNumbers = param.find( | ||
| p => p.name === 'withoutNumbers' | ||
| p => p.label === 'withoutNumbers' |
There was a problem hiding this comment.
[F] For changes of name to label see this.
| column => | ||
| column.path === | ||
| joinColumnPath(ColumnType.PARAMS, 'params.yaml', 'mixedNumber') | ||
| buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml', 'mixedNumber') |
There was a problem hiding this comment.
[F] We now have different builders for Params and Metrics and Deps due to the different data structures. Hence the rename here (and throughout the PR).
| collectDepChanges(changes, workspaceData, commitData) | ||
| } | ||
|
|
||
| export const collectParamsFiles = ( |
There was a problem hiding this comment.
[F] This has just been relocated unchanged from extension/src/experiments/columns/collect.ts.
| @@ -0,0 +1,93 @@ | |||
| import { join } from 'path' | |||
There was a problem hiding this comment.
[F] In order to accommodate the split in logic between metrics & params and deps I moved extension/src/experiments/columns/collect.ts. => extension/src/experiments/columns/collect/index.tsand created extension/src/experiments/columns/collect/deps.ts, extension/src/experiments/columns/collect/metricsAndParams.ts extension/src/experiments/columns/collect/util.ts (shared helpers) files. This has added significantly to the diff here.
| return changes.sort() | ||
| } | ||
|
|
||
| export const collectParamsFiles = ( |
There was a problem hiding this comment.
[F] This has just been relocated unchanged from extension/src/experiments/columns/collect.ts.
| } | ||
|
|
||
| collectMetricAndParamChanges(changes, workspace, currentCommit) | ||
| collectDepChanges(changes, workspace, currentCommit) |
There was a problem hiding this comment.
[F] This is new logic. The rest is unchanged.
| parentPath: string, | ||
| type: ColumnType | ||
| ) => { | ||
| if (!acc[path]) { |
There was a problem hiding this comment.
[F] This behavioral change will save us a significant amount of cycles. If we've seen the path before we've already walked all of its children so we can't gain anything by walking it again.
| ) => { | ||
| const [path, ...rest] = ancestors | ||
| /* | ||
| The depth is only limited for the middle of the path array. |
There was a problem hiding this comment.
[F] Updated this comment because I am not passing more to the function but it actually confused me more having this here.
| dvcRoot, | ||
| workspaceState, | ||
| PersistenceKey.METRICS_AND_PARAMS_STATUS, | ||
| splitColumnPath |
There was a problem hiding this comment.
[F] We are now adding the label to each element instead of having this function (it is no longer as simple to get this value from the path). Overall makes the base class cleaner.
| [join('src', 'evaluate.py')]: '44e714021a65edf881b1716e791d7f59', | ||
| [join('src', 'featurization.py')]: 'e0265fc22f056a4b86d85c3056bc2894', | ||
| [join('src', 'prepare.py')]: '935ee6803ac617d0ef138ac33a9e9a77', | ||
| [join('src', 'train.py')]: 'c3961d777cfbd7727f9fde4851896006' |
There was a problem hiding this comment.
[F] This is the data structure that we're running with for deps for now.
mattseddon
force-pushed
the
add-deps-to-experiment-table
branch
from
06cbf43 to
d81a2f2
Compare
mattseddon
requested review from
maxagin,
rogermparent,
shcheklein,
sroy3 and
wolmir
mattseddon
changed the title
|
Cool stuff, @mattseddon ! Questions / followups (at least keep tickets open):
|
We show them all, as per #1183 (comment)
I can change to file size. Currently when the hash is changed against the HEAD it will be highlighted (same as for all params and metrics). E.g with train.py changed:
I incorrectly copied the CLI, they show the first 7 characters of the hash:
Can we copy the CLI to get this in and address all changes via #1657? |
|
The |
Yep, let's just keep the relevant tickets open please. We need to the next step as part of the story.
yep, let's show size by default, but also show some yellow dot nearby to highlight the changes (dot itself might be even more useful vs size and hash to be honest)
yep |
Updated and added a relevant comment in #1657. |
|
Code Climate has analyzed commit 3fd7fa9 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 98.9% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.9% (0.0% change). View more on Code Climate. |
3 participants
Closes #1183.
Closes #1768.
Relates to #1657.
This PR may seem gigantic at first glance but the file count is significantly contributed by the name change of
joinColumnPathand also renaming a variable in the columns data (name=>label). The diff is largely accounted for by changes to the test fixture and tests.Demo
Screen.Recording.2022-06-06.at.9.22.51.am.mov
(plots not broken)
(colors)
Screen.Recording.2022-06-06.at.9.33.55.am.mov