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

Bump min DVC version to 2.52.0 (plots errors) #3477

Merged
merged 5 commits into from
Apr 2, 2023
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Mar 16, 2023

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

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

TODOs:

@@ -114,21 +109,15 @@ suite('Experiments Tree Test Suite', () => {

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
Member 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
Member

Confirm errors API changes give everything needed.

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

@mattseddon
Copy link
Member 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
Member

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
Member

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
Member 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
Member

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

@mattseddon
Copy link
Member 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
Member 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
Member

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

demo Outdated
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
Member 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
@codeclimate
Copy link

codeclimate bot commented Apr 2, 2023

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