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

DependencyFix/Upgrade ome-types to 0.3.4 #522

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

Armavica
Copy link
Contributor

@Armavica Armavica commented Jul 6, 2023

Description

In version 0.3.4, ome-types restricts pydantic to < 2.0.

This upgrade prevents aicsimageio from importing ome-types 0.3.3, which will crash with pydantic 2.0 (released yesterday), making aicsimageio unusable.

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • Link to any relevant issue in the PR description. Ex: Resolves [gh-], adds tiff file format support
  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

In version 0.3.4, ome-types restricts pydantic to < 2.0.

This upgrade prevents aicsimageio from importing ome-types 0.3.3, which
will crash if pydantic 2.0 is installed, making aicsimageio unusable.
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.05% ⚠️

Comparison is base (2e88f68) 94.03% compared to head (5f6b8ea) 93.98%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
- Coverage   94.03%   93.98%   -0.05%     
==========================================
  Files          53       53              
  Lines        4659     4658       -1     
==========================================
- Hits         4381     4378       -3     
- Misses        278      280       +2     
Files Changed Coverage Δ
aicsimageio/tests/readers/test_ome_tiff_reader.py 95.45% <ø> (ø)
aicsimageio/tests/writers/test_ome_tiff_writer.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Collaborator

fwiw, ome-types v0.4.0 will also be compatible with pydantic2. (not that this shouldn't still be merged)

@Armavica
Copy link
Contributor Author

Great! Then I let you decide what makes more sense, considering aicsimageio's release schedule. In the meantime, I had to add the restriction pydantic < 2.0 in my library, in order to avoid the incompatibility with ome-types 0.3.3, and it fixed the issue on my side. So it is not a problem for me to wait for a release of aicsimageio with ome-types 0.4.0 if you think that makes more sense.

@tlambert03
Copy link
Collaborator

merge?

@toloudis
Copy link
Collaborator

The last commit caused build breakage. It changes dependency versions.

@toloudis
Copy link
Collaborator

I may have got the merge wrong. The only change we want here is ome types >= 0.3.4 correct?

@tlambert03
Copy link
Collaborator

yeah, that should work

@toloudis
Copy link
Collaborator

Will make sure tests pass and then merge asap

@toloudis
Copy link
Collaborator

toloudis commented Aug 14, 2023

I'm wondering if 0.3.4 raises a different exception than 0.4.0+ for trying to parse "bad ome string".

@tlambert03
Copy link
Collaborator

ah, yes that's possible. do you need to be asserting the specific exception type of an external library? (if it changed the behavior of your usage, it would make sense, but here it kinda just seems like you're testing an external library)

@toloudis
Copy link
Collaborator

toloudis commented Aug 15, 2023

Cleaning this up revealed one more test that has different results in ome-types 0.3.4 vs 0.4.1. This now has to do with our ome-tiff metadata parsing and where we detect whether or not we need to do our own auto-cleanup. I'm tempted to just remove the test cases for this since they fail (expectedly) for 0.3.4 and pass (unexpectedly) for 0.4.1.
@SeanLeRoy @BrianWhitneyAI @evamaxfield What do you think? This is in test_ome_tiff_reader.py test_known_errors_without_cleaning .

@tlambert03
Copy link
Collaborator

Yeah I also noticed that in my tests... that aicsimagio was essentially asserting that something would be an error forever 😀

Lots of things were improved that will probably ultimately remove your need for the special cleanup.

I agree just remove the cases.

@toloudis
Copy link
Collaborator

toloudis commented Aug 15, 2023

I agree just remove the cases.

Gonna do it. 😀

@evamaxfield
Copy link
Collaborator

Fully supportive of removing the tests! I think I remember when those tests were added and they were around the same time as our work on fixing some bad metadata cases and such iirc. Thanks for all the work and thoughts on this everyone!

@toloudis toloudis merged commit 1a67c50 into AllenCellModeling:main Aug 16, 2023
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants