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.

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

Delay creation of plots to remove optional logic #2832

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Nov 25, 2022

This is a precursor to the work being done in #2831. We can simplify some of the logic in Plots and PlotsData by delaying the creation of plots repositories until after Experiments have been created for each dvc root.

this.dvcRoots,
this.updatesPaused,
this.resourceLocator,
this.experiments
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] By creating this after we have created the experiments repositories we can simply/remove a lot of the optional chaining logic from Plots and PlotsData (model is passed in on instantiation instead of being set afterwards).

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mattseddon mattseddon force-pushed the instantiate-plots-with-experiments branch from d3a043c to c6455f7 Compare November 25, 2022 07:07
@mattseddon mattseddon marked this pull request as ready for review November 25, 2022 07:10
Copy link
Contributor

@sroy3 sroy3 left a comment

Choose a reason for hiding this comment

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

On my end, loading plots can take a little while (mainly because that's usually the first thing I open when launching the extension). Wondering if the message should say more than just "Loading Plots..." If we are delaying their creation + already long loading time.

@mattseddon
Copy link
Member Author

On my end, loading plots can take a little while (mainly because that's usually the first thing I open when launching the extension). Wondering if the message should say more than just "Loading Plots..." If we are delaying their creation + already long loading time.

Technically this change shouldn't hurt performance as we have to wait for experiments to be built before we know which revisions to request. This is just a simpler/smarter way to structure the code.

@mattseddon mattseddon enabled auto-merge (squash) November 25, 2022 22:52
@codeclimate
Copy link

codeclimate bot commented Nov 25, 2022

Code Climate has analyzed commit 142152d and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 92.5% (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 mattseddon merged commit 9706efc into main Nov 25, 2022
@mattseddon mattseddon deleted the instantiate-plots-with-experiments branch November 25, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants