Skip to content

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Feb 1, 2022

This PR provides live updates to static plots for repositories that are using dvclive without checkpoints and running experiments within the workspace (--temp scenario not covered).

Addresses part of #1256

Demo (non-checkpoints)

Screen.Recording.2022-02-02.at.4.06.33.pm.mov

The above highlights the difference in speed between exp show and plots diff, we can see that the plots are updated well before the experiments data.

Demo (checkpoints)

Screen.Recording.2022-02-02.at.4.18.35.pm.mov

For follow up:

  1. If an experiment is running and we unselect the experiment then we get unexpected behaviour in the plots because we use getSelectedRevisions to determine which revisions are missing.
  2. Timing issue on the first iteration of a checkpoint experiment -> workspace comes through the watcher and then is quickly followed by the experiment revision (looks like only solution is to parse the dvc.yaml and see if the repo has checkpoints, if it does not use the watcher whilst experiments are running).

@mattseddon mattseddon added the product PR that affects product label Feb 1, 2022
@mattseddon mattseddon self-assigned this Feb 1, 2022
import { reportError } from '../vscode/reporting'

export const autoRegisteredCommands = {
EXPERIMENT_IS_RUNNING: 'isRunning',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] We need this because we don't want to call for updates when there are changes coming through the watchers, a checkpoints experiment is running and there are no missing revisions (all revisions are immutable).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could rename it something like EXPERIMENT_CHECK_IS_RUNNING: 'checkIsRunning'. It is longer, but it doesn't look out of place in the commands list.

@mattseddon mattseddon marked this pull request as ready for review February 2, 2022 05:21
@mattseddon mattseddon marked this pull request as draft February 2, 2022 05:21
@mattseddon mattseddon marked this pull request as ready for review February 2, 2022 05:29
(await this.internalCommands.executeCommand<boolean>(
AvailableCommands.EXPERIMENT_IS_RUNNING
)) &&
!definedAndNonEmpty(revisions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] For follow up need to find a way to distinguish between checkpoint and non-checkpoint repositories to get rid of the workspace timing issue described in the description of this PR.

import { reportError } from '../vscode/reporting'

export const autoRegisteredCommands = {
EXPERIMENT_IS_RUNNING: 'isRunning',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could rename it something like EXPERIMENT_CHECK_IS_RUNNING: 'checkIsRunning'. It is longer, but it doesn't look out of place in the commands list.

Comment on lines 167 to 176
return uniqueValues([
...this.getSelectedRevisions().filter(
rev =>
!uniqueValues([
...Object.keys(this.comparisonData),
...Object.keys(this.revisionData),
'workspace'
]).includes(rev)
)
])
Copy link
Contributor

@rogermparent rogermparent Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason the array passed to uniqueValues needs to be duplicated? I don't think the Set constructor mutates its passed in array, and filter also makes a shallow copy.

Probably not a big deal though.

Copy link
Contributor Author

@mattseddon mattseddon Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly because of this: treeverse/dvc#7265.

This logic does need cleaned up. I will follow up.

@mattseddon mattseddon force-pushed the update-non-checkpoint-plots branch from e5940b0 to d37bf40 Compare February 2, 2022 21:36
@mattseddon mattseddon enabled auto-merge (squash) February 2, 2022 21:36
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit d37bf40 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 96.4% (0.1% change).

View more on Code Climate.

@mattseddon mattseddon merged commit e1f0dc5 into master Feb 2, 2022
@mattseddon mattseddon deleted the update-non-checkpoint-plots branch February 2, 2022 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants