Pydantic 2 upgrade #264
Pydantic 2 upgrade #264
Conversation
|
The test failure is an http timeout, I think this is ready for review. I'll try re-running the remote_data test later today. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
- Coverage 69.89% 69.74% -0.16%
==========================================
Files 25 24 -1
Lines 2966 2938 -28
==========================================
- Hits 2073 2049 -24
+ Misses 893 889 -4 ☔ View full report in Codecov by Sentry. |
mwcraig
force-pushed
the
pydantic-2-upgrade
branch
from
e598225 to
85b8338
Compare
This only sort-of works, but can do Time and SkyCoord, which is what we need immediately.
This is necessary for them to be able to used in type unions like
Annotated[QuantityType, EquivalentTo("second")] | None
mwcraig
force-pushed
the
pydantic-2-upgrade
branch
from
3cb6419 to
e7d52fe
Compare
| def validate_by_instantiating(source_type): | ||
| """ | ||
| Return a function that tries to create an instance of source_type from a value. | ||
| The intended use of this is as a vallidotr in pydantic. |
There was a problem hiding this comment.
I assume you mean "validator" and not "vallidotr".
| # since the first thing in the chain has to handle the value coming | ||
| # from json,while the last thing generates the python value for the | ||
| # input. With the choice below we will *always* want`mode="validation"` | ||
| # because pydantic cannot generate a schema fora Unit or Quantity. |
| return hash(self.physical_type) | ||
|
|
||
|
|
||
| # We have lost default titles and exmples, but that is maybe not so bad |
|
|
||
| # We have lost default titles and exmples, but that is maybe not so bad | ||
|
|
||
| # This is really nice compared to pydantiv v1... |
|
|
||
| In principle, we ought to be able to use the astropy serialization stuff | ||
| that is used in writing tables to ecsv here, but that is not quite working | ||
| yet. |
There was a problem hiding this comment.
Just an opinion, I don't like leaving "dated" comments that might be forgotten without actually dating them. eg. "As of January 2024, this is not quite working." In general, I understand the need to comment on an incomplete feature, but I can imagine forgetting to update it without something like a date to stand out in the vision while reviewing the code.
| gap: conint(ge=1) = Field(autoui=CustomBoundedIntTex, default=1) | ||
| annulus_width: conint(ge=1) = Field(autoui=CustomBoundedIntTex, default=1) | ||
| # model_config = MODEL_DEFAULT_CONFIGURATION | ||
|
|
There was a problem hiding this comment.
For the pydantic uninitiated, should we add a comment saying we are restricting radius, gap, and annulus_width to be integers with a value greater than 1 in the docstring (lines 218-224)?
There was a problem hiding this comment.
I made two changes here that you'll see when I push the code in a bit:
- Add notes to the docstrings indicating values must be positive
- Used
PositiveIntandPositiveFloatfrom pydantic 2 instead ofconint(ge=1)andconfloat(ge=0). The former are much less opaque than the latter
| gap: conint(ge=1) = Field(autoui=CustomBoundedIntTex, default=1) | ||
| annulus_width: conint(ge=1) = Field(autoui=CustomBoundedIntTex, default=1) | ||
| # model_config = MODEL_DEFAULT_CONFIGURATION | ||
|
|
There was a problem hiding this comment.
I think you should add a comment for the pydantic uninitiated that these lines ensure radius, gap, annulus_width should all be integers greater than or equal to 1.
| "init", | ||
| [ | ||
| Unit("m"), | ||
| 1, |
There was a problem hiding this comment.
Can you test with a float as well and a float with a unit? Yeah, a little silly, but trying to be through.
There was a problem hiding this comment.
What do you mean by "float with a unit"? I think 5 * Unit('m') is already guaranteed to be a Unit so it is covered by the Unit("m") case.
There was a problem hiding this comment.
What do you mean by "float with a unit"? I think
5 * Unit('m')is already guaranteed to be aUnitso it is covered by theUnit("m")case.
Oh, I still have this idea that 5 * Unit('m') is different from 5.0*Unit('m') since Astropy values with units I thought got stored as numerical type + unit information. If I am incorrect, forget it.
There was a problem hiding this comment.
Here is what I wrote down needed to be tested BEFORE reviewing your tests (after reviewing the rest of the code) along with my comments after reviewing your tests
- Make sure both Unit and Quantity can be created from a string or a float or an
instance of the same type. So we need to check for all of those cases. [MOSTLY
DONE, missing float tests I think] - Test use of
UnitType[Not tested directly, but as part of above calls] - Test use of
QuantityType[Not tested directly, but as part of above calls] - Test use of
EquivalentTo[TESTED, including validation failures] - Test use of
WithPhysicalType[TESTED, including failures] - Test use of
AstropyValidator[TESTED, but MISSING tests for validation failures]
The only thing I really think is missing from the tests are exploits tests for floats with Quantity and Unit creation and the confirmation that expected validation failures occur with AstropyValidator.
|
BTW, I understand the need to use the private AstroPy API to serialize astropy types. I like how you wrapped it with another function. Any chance we can ask AstroPy to expose _represent_as_dict() in a PR, since other people using pydantic 2.x may want to use this. In fact, I am wondering if astropy_pydantic.py could be released as a separate library for others, since it could be helpful to other projects using astropy. |
| "init", | ||
| [ | ||
| Unit("m"), | ||
| 1, |
There was a problem hiding this comment.
What do you mean by "float with a unit"? I think
5 * Unit('m')is already guaranteed to be aUnitso it is covered by theUnit("m")case.
Oh, I still have this idea that 5 * Unit('m') is different from 5.0*Unit('m') since Astropy values with units I thought got stored as numerical type + unit information. If I am incorrect, forget it.
Forgot to respond to this -- if we can figure out how to translate the YAML schema astropy has to a JSON schema then I think it would make sense to release a separate package. I may ask in slack this week about how to do that. The intent in astropy right now is that you load |
This is a pretty big one. I'd recommend focusing on the tests and whether they seem to reasonably cover the code if you don't want to dive into the details of pydantic 2.
Although it was a bit of a slog, pydantic 2 makes it much easier to handle astropy types by writing type annotators (see the new docstrings and tests).
One thing that I know isn't quite right yet is tapping into the serialization that astropy can already do to Table and asdf. I found and am using the
_represent_as_dictmethod to do an initial pass at this that will work for now for the types we need.@adonath -- I thought this might be of interest to gammapy.
@JuanCab -- if you could aim for Monday or Tuesday that would be great -- I'll keep working on unrelated issues and/or base new pull requests off this one if needed.
Edit: Closes #262