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

Jump to bottom

Use linear path to update plots data when experiments update #2831

Closed
wants to merge 7 commits into from
Closed

Use linear path to update plots data when experiments update #2831

wants to merge 7 commits into from

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Nov 25, 2022 •
edited
Loading

WIP

@mattseddon mattseddon self-assigned this Nov 25, 2022
mattseddon
mattseddon commented Nov 25, 2022
View reviewed changes
extension/src/plots/model/index.ts
@@ -105,6 +106,10 @@ export class PlotsModel extends ModelWithPersistence {
}

public async transformAndSetPlots(data: PlotsOutput, revs: string[]) {
if (data.error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Related to this comment: #1649 (comment). Will fix separately.

This was referenced Nov 25, 2022
Merged
Closed
@mattseddon mattseddon changed the base branch from main to fix-multi-source-plots-data November 27, 2022 22:49
codeclimate[bot]
codeclimate bot reviewed Nov 27, 2022
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26259 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon changed the base branch from fix-multi-source-plots-data to main November 27, 2022 23:02
codeclimate[bot]
codeclimate bot reviewed Nov 27, 2022
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26272 lines exceeds the maximum allowed for the inline comments feature.

codeclimate[bot]
codeclimate bot reviewed Nov 27, 2022
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26260 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon changed the base branch from main to fix-multi-source-plots-data November 27, 2022 23:19
mattseddon
mattseddon commented Nov 27, 2022
View reviewed changes
extension/src/plots/index.ts
@@ -48,7 +49,7 @@ export class Plots extends BaseRepository<TPlotsData> {
this.experiments = experiments

this.plots = this.dispose.track(
new PlotsModel(this.dvcRoot, experiments, workspaceState)
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Removing experiments from here so that we can't get into a race condition where the selected revisions change mid-update.

mattseddon
mattseddon commented Nov 27, 2022
View reviewed changes
extension/src/plots/model/index.ts

this.fetchedRevs = new Set([
...this.fetchedRevs,
...revs.map(rev => cliIdToLabel[rev])
...newRevs.map(rev => cliIdToLabel[rev])
])
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] I think we can expand fetchedRevs to include state information about each revision. We can then display this information on the ribbon. The 4 states would be:

  • loading
  • loaded with data
  • loaded no data
  • loaded with errors

We could potentially combine 3 & 4.

Copy link
Member Author

Choose a reason for hiding this comment

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

WDYT?

@mattseddon mattseddon changed the base branch from fix-multi-source-plots-data to main November 28, 2022 01:37
@mattseddon mattseddon changed the base branch from main to fix-multi-source-plots-data November 28, 2022 01:38
codeclimate[bot]
codeclimate bot reviewed Nov 28, 2022
View reviewed changes
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26285 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

codeclimate bot commented Nov 28, 2022

Code Climate has analyzed commit 44aed94 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 96.6% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@mattseddon
Copy link
Member Author

I've been chasing shadows in this PR. The specific issue that I was trying to solve is checkpoints being updated too quickly to ever be shown inside of the plots webview. This happens when there is not much data in the experiments table + checkpoints are created in a rapid manner. A better solution for that particular issue has been proposed in #2424.

I am going to take a different approach to speed up the ribbon being loaded.

@mattseddon mattseddon closed this Nov 28, 2022
@mattseddon mattseddon deleted the stabilise-plots branch November 28, 2022 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

Assignees

@mattseddon mattseddon

Labels
🏠 housekeeping
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mattseddon