Skip to content

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Mar 16, 2023

1/4 main <- this <- #3520 <- #3522 <- #3532

This PR accommodates the breaking part of the change in treeverse/dvc#9146.

TODOs:

@mattseddon mattseddon self-assigned this Mar 16, 2023
@mattseddon mattseddon added do not merge A: integration Area: DVC integration layer labels Mar 16, 2023
expect(
messageSpy,
'when there are no experiments selected we dont send checkpoint type plots'
"when there are no experiments selected we don't send checkpoint trend plots"
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] By default we now give the plots diff fixture instead of nothing. The test only needs to check the match assumption

@skshetry
Copy link
Collaborator

Confirm errors API changes give everything needed.

Hi, @mattseddon. Are you able to confirm if errors give you everything?

@mattseddon
Copy link
Contributor Author

Confirm errors API changes give everything needed.

Hi, @mattseddon. Are you able to confirm if errors give you everything?

It looks very likely. I am working through the last couple of parts now.

The one thing that was missing (which I am not sure if you will be able to provide), is the plot type for each error ('image' or 'vega'). Can you add that in?

@skshetry
Copy link
Collaborator

The one thing that was missing (which I am not sure if you will be able to provide), is the plot type for each error ('image' or 'vega'). Can you add that in?

Is plot_id not enough to figure that out? I am trying to think of when this can happen. The only one that I can think of is when the plot images are missing.

We choose between vega and image based on file extensions. So:

  1. either the images' directory is missing, and we don't know what contents are (which makes it fallback to vega plots).
  2. or, a image file is missing, which will still be considered as image plot but won't have any data.

In both of these cases, there will be no data and hence no type in the json output.
We can say type is "image" for 2nd kind of error, but 1st one is still misleading.

@skshetry
Copy link
Collaborator

But this is of course in case where there is no plots' data at all. In those cases, do you need to differentiate if it's a vega or an image plot?

@mattseddon
Copy link
Contributor Author

But this is of course in case where there is no plots' data at all. In those cases, do you need to differentiate if it's a vega or an image plot?

When all we have is errors for a plot then we don't know the type. If we don't know the type then we don't know what section it should be displayed in. If it should be an image then we can't provide a row in the image comparison table. It's an edge case and we can live without the data. It's ok not to worry about it.

🙏🏻

@skshetry
Copy link
Collaborator

@mattseddon, what do you think of supporting older dvc versions? Do you think that's possible?

@mattseddon
Copy link
Contributor Author

@mattseddon, what do you think of supporting older dvc versions? Do you think that's possible?

If we had a lot more users and there were legitimate reasons that they could not update the CLI then I would consider attempting to make things backwards compatible but at the moment it is not worth the effort IMO.

@mattseddon
Copy link
Contributor Author

Give me a heads-up when you're going to make the release and I'll make the corresponding extension release 🙏🏻.

@mattseddon mattseddon marked this pull request as ready for review March 24, 2023 03:51
@skshetry
Copy link
Collaborator

Okay, we’ll merge and release early next week, either Mon or Tue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but that branch contains the breaking change. I have to wait for a release before I can update it. There is a checkbox on the description to update 🙏🏻.

@mattseddon mattseddon changed the title Accomodate breaking change in plots diff Update min version of DVC to 2.52.0 (plots diff breaking change) Apr 1, 2023
@mattseddon mattseddon changed the title Update min version of DVC to 2.52.0 (plots diff breaking change) Bump min DVC version to 2.52.0 (plots errors) Apr 1, 2023
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 6fdc993 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 95.1% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon enabled auto-merge (squash) April 2, 2023 01:29
@mattseddon mattseddon merged commit e345fce into main Apr 2, 2023
@mattseddon mattseddon deleted the jump-plots-diff-bc branch April 2, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: integration Area: DVC integration layer 🏠 housekeeping

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants