Support pydantic v2.0 #4750

adonath · 2023-09-15T15:31:54Z

Description
This pull request starts the refactoring to support Pydantic v2.0. I'm running into many issues, but mostly because the Gammapy code base is very inconsistent. Pydantic complains correctly most of the time. I'll start collecting issues here...

Update:

Gammapy used None as default in many place, without declaring it as allowed type. I change to use Optional in these cases. I think we should rather change to more meaningful default values, bot not in this PR.
I consistently changed to a new introduce PathType, which does a minimal validation based on make_path
I had to change to arbitrary_types_allowed=True for now, because in Pydantiv v2.0 there is now support for __get_validators__. This would need to be supported using __get_pydantic_core_schema__ (https://docs.pydantic.dev/latest/usage/types/custom/#customizing-validation-with-__get_pydantic_core_schema__) instead. Which I consider to much work for now.
Beside the incompatibility with ray the transition seems to work.

adonath · 2023-09-15T18:26:05Z

Currently ray only supports pydantic<2.0 (https://github.com/ray-project/ray/blob/master/python/setup.py#L265), we cannot update right now. We have to wait for ray to update as well...

adonath · 2023-09-18T14:02:16Z

Corresponding issue in ray: ray-project/ray#39722

dioptre · 2023-09-25T18:44:45Z

Thanks for this. Probably many people waiting on it.

adonath · 2023-11-03T15:28:14Z

The CI still fails here because of ray. Interestingly it works for me locally, so I can get succeeding test with ray[default] == 2.8.0 and pydantic=2.4.2. On closer inspection the versions are not compatible, because ray still requires pydantic < 2.0, but accidentally it seems to work. Not sure what to do about this...

adonath · 2024-01-10T17:47:26Z

I rebased and updated to ray 2.9. This should be ready to review now. Here are some questionable behaviors I found:

I think converting None to SkyCoord(np.nan, np.nan) is not a good idea. Either the object is optional or it is not.
In config.py we rely on the default values being defined, this required setting validate_defaults=True, even if the default is None, which make the value optional. I think we should replace them with meaningful defaults and probably use validate_defaults=False again.

adonath · 2024-01-10T17:48:54Z

I still have to fix some places that raise deprecation warnings...

adonath · 2024-01-11T14:43:08Z

The remaining test fails seems unrelated...

maxnoe · 2024-01-11T14:49:15Z

I think converting None to SkyCoord(np.nan, np.nan) is not a good idea. Either the object is optional or it is not.

I fully agree. We have a similar issue in the data model of pointing I think, where for some reason it is required to provide a ra/dec pointing but SkyCoord(nan, nan) is allowed

bkhelifi · 2024-01-11T15:39:44Z

gammapy/utils/metadata.py

-            raise ValidationError(
-                f"Incorrect position. Expect SkyCoord got {type(v)} instead."
+            raise ValueError(
+                f"Incorrect position. Expect SkyCoord in altaz frame got {type(v)} instead."


the frame should be in icrs, and not alt/az... Maybe a copy/paste typo?

setup.cfg

maxnoe · 2024-01-11T16:14:15Z

gammapy/utils/types.py

+    BeforeValidator(validate_angle),
+]
+
+EnergyType = Annotated[


Astropy Quantity supports generic typing over the physical quantity, maybe that's a better solution than individual definitions?

In [1]: from astropy.units import Quantity In [2]: Quantity['length'] Out[2]: typing.Annotated[astropy.units.quantity.Quantity, PhysicalType('length')] In [3]: Quantity['energy'] Out[3]: typing.Annotated[astropy.units.quantity.Quantity, PhysicalType({'energy', 'torque', 'work'})] In [4]: Quantity['angle'] Out[4]: typing.Annotated[astropy.units.quantity.Quantity, PhysicalType('angle')]

Thanks, I was not aware of this. However in our case, we mostly do it for the custom validation and serialization methods and only the energy type is handled with a quantity object. I guess Pydantic does not understand the PhysicalType annotation, so we need a validator in any case.

This seems to work nicely:

from typing import Annotated, get_args from pydantic import BaseModel, BeforeValidator, ConfigDict, ValidationError from pydantic.functional_validators import AfterValidator import astropy.units as u from functools import partial def validate_physical_type(value: u.Quantity, physical_type: u.PhysicalType): if value.unit is None: raise ValueError(f"Expected physical_type: {physical_type}, but value had no unit") if (t := value.unit.physical_type) != physical_type: raise ValueError(f"Expected physical_type: {physical_type}, got {t}") return value class ValidatedQuantity(u.Quantity): @classmethod def __class_getitem__(cls, physical_type: str): q = super().__class_getitem__(physical_type) validate = partial(validate_physical_type, physical_type=get_args(q)[1]) return Annotated[ q, BeforeValidator(lambda v: cls(v, copy=False)), AfterValidator(validate) ] class Foo(BaseModel): model_config = ConfigDict(arbitrary_types_allowed=True) length : ValidatedQuantity['length'] = 5 * u.m Foo(length=5 * u.cm) try: Foo(length=5 * u.deg) except ValidationError as e: print(e)

Thanks this looks generally useful! I think for this PR it does not add any value, because currently we only have the EnergyType. But I'm pretty sure we will need it in future, so maybe you just want to contribute it in a follow up PR yourself?

Extending it for an optional required shape might also be interesting, such that the following works:

class Foo(BaseModel): model_config = ConfigDict(arbitrary_types_allowed=True) arbitrary_length: ValidatedQuantity['length'] = [[5, 5], [3, 3]] * u.m scalar_length : ValidatedQuantity['length', ()] = 5 * u.m other_length: ValidatedQuantity['length', (2,)] = [5, 3] * u.m

When no shape is given, no validation is performed.

Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>

registerrier

Thanks @adonath . This looks really nice.

Regarding SkyCoord(np.nan, np.nan) as empty values returned by the validator, I think the point is to avoid errors when accessing some information like this. It also allows easy concatenation of such coordinates when stacking metadata at DL4.

For extra_allow=False, the main point is how to allow for flexibility for metadata class additions for specific experiments/observatories. I was assuming that we could allow addition of any Metadata derived object with its specific FITS serialization scheme. Could there be a way to allow only extra MetaData objects?

registerrier · 2024-01-11T17:41:16Z

gammapy/analysis/config.py

@@ -169,8 +131,8 @@ class ExcessMapConfig(GammapyBaseConfig):


 class BackgroundConfig(GammapyBaseConfig):
-    method: BackgroundMethodEnum = None
-    exclusion: Path = None
+    method: Union[BackgroundMethodEnum, None] = None


why not Optional here?

Thanks , I think I just forgot to use Optional here...

registerrier · 2024-01-11T17:45:56Z

gammapy/data/metadata.py

-    pointing: Optional[PointingInfoMetaData]
-    target: Optional[TargetMetaData]
-    location: Optional[Union[EarthLocation, str]]
+    _tag: ClassVar[Literal["observation"]] = "observation"


What does this do?

This does two things:

_tag is currently not part of the model definition, because by convention Pydantic treats attributes starting with underscore as private attributes. This is also why the ClassVar is needed. The tag is assigned to the class not the instance.

The Literal only allows exactly this value. However here it has not a real function, except for documentation.

I would actually propose to make the tag part of the model definition and use the Literal. This will fix the value to the value defined by the Literal.

registerrier · 2024-01-11T17:49:39Z

gammapy/utils/metadata.py

+    model_config = ConfigDict(
+        arbitrary_types_allowed=True,
+        validate_assignment=True,
+        extra="forbid",


Would it be possible to allow specific types, i.e. MetaData derived entries?

As far as I know this is not possible. We would need to work with a container such as List[MetaData] or Dict[str, MetaData] to allow for the type validation.

registerrier · 2024-01-11T17:51:40Z

gammapy/utils/metadata.py

-        return cls(creator=creator, date=date)
+    _tag: ClassVar[Literal["creator"]] = "creator"
+    creator: Optional[str] = f"Gammapy {version}"
+    date: Optional[TimeType] = Field(default_factory=Time.now)


So here default_factory is a function that generates the default value? Hence the now instead of now()?

Yes, exactly! This is the recommended Pydantic pattern for default factories.

registerrier · 2024-01-11T17:53:46Z

gammapy/utils/metadata.py

-    instrument: Optional[str]
-    sub_array: Optional[str]
-    observation_mode: Optional[str]
+    obs_id: int


I was wondering whether some formats would provide IDs that are not ints. It is probably simpler to assume a unique type indeed

Yes, I was confused by this. We could certain support both if needed. But right now I'm not aware of any data that would require the string.

registerrier · 2024-01-11T17:57:31Z

gammapy/utils/metadata.py

-        if v is None:
-            return SkyCoord(np.nan, np.nan, unit="deg", frame="icrs")
-        elif isinstance(v, SkyCoord):
+    @field_validator("position", mode="after")


Why is the mode after here?
Why reimplementing validate_radec_mean? Rather call it differently no?

registerrier · 2024-01-11T17:58:32Z

gammapy/utils/metadata.py


-    radec_mean: Optional[SkyCoord]
-    altaz_mean: Optional[Union[SkyCoord, AltAz]]
+    radec_mean: Optional[SkyCoord] = None


Would it be practical to create a SkyCoordICRSType and SkyCoordAltAzType?

Yes, this would be more elegant. I can introduce those.

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

MRegeard · 2024-01-12T15:50:36Z

Don't we also need to update the environment-dev.yaml file, which currently says pydantic<2.0 ?

bkhelifi · 2024-01-12T15:56:14Z

Don't we also need to update the environment-dev.yaml file, which currently says pydantic<2.0 ?

And the codemeta also

adonath · 2024-01-12T16:39:40Z

The last remaining test fail was related to the fact that we use Time objects with multiple values, instead of scalar. I think we should actually make explicit in future when we allow scalar values and when arrays. See the comment #4750 (comment), I made to @maxnoe 's suggestion.

Thanks @bkhelifi and @MRegeard, I also updated the meta and conda env file.

codecov · 2024-01-12T16:59:44Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (315f0ca) 75.69% compared to head (4369788) 75.69%.
Report is 13 commits behind head on main.

❗ Current head 4369788 differs from pull request most recent head 16d849d. Consider uploading reports for the commit 16d849d to get more accurate results

Files	Patch %	Lines
gammapy/utils/types.py	89.74%	8 Missing ⚠️
gammapy/analysis/core.py	0.00%	1 Missing ⚠️
gammapy/estimators/points/sed.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4750   +/-   ##
=======================================
  Coverage   75.69%   75.69%           
=======================================
  Files         228      229    +1     
  Lines       33841    33836    -5     
=======================================
- Hits        25616    25613    -3     
+ Misses       8225     8223    -2

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

adonath · 2024-01-12T17:05:51Z

@registerrier @QRemy PR is ready to merge from my side...

registerrier

Thanks @adonath . This looks good.

there is a remaining failure visible in the tutorials. Could it be due to this definition that is not supported in the current scheme:

gammapy/examples/tutorials/analysis-2d/modeling_2D.py

Line 95 in 0dd7399

config.datasets.safe_mask.parameters = {"offset_max": 2.5 * u.deg}

Since there is no generic Quantity type defined with its serializer this has to fail when printing config .

adonath · 2024-01-15T20:46:44Z

@registerrier This is due to the generic parameters container. Those values are not validated and cannot really be serialized safely. Users can put arbitrary things and it will always fail...I think we should deprecate the parameters. The duplication of the maker config is maybe the lesser evil.

registerrier

Thanks @adonath . This looks good! No further comment.

adonath · 2024-01-16T21:32:11Z

@maxnoe or @bkhelifi please approve or make further comments. Thanks!

bkhelifi

Thanks a lot @adonath for this important upgrade!

adonath · 2024-01-17T13:23:22Z

Thanks @registerrier and @bkhelifi, I'll go ahead and merge now.

adonath force-pushed the support_pydantic_v2.0 branch from 8ed2411 to 95ec247 Compare September 15, 2023 15:42

chaimain mentioned this pull request Sep 17, 2023

Resolve the migration to latest pydantic and autodoc_pydantic chaimain/asgardpy#116

Closed

adonath mentioned this pull request Nov 3, 2023

Introduce hierarchical metadata structures #4886

Merged

adonath force-pushed the support_pydantic_v2.0 branch 2 times, most recently from 3a9d708 to 1aea6f6 Compare November 3, 2023 15:16

adonath added 2 commits January 10, 2024 10:59

Require pydantic >=2.5.0 and ray>=2.9

66a36b4

Update to Pydantic v2

f7aed3d

adonath force-pushed the support_pydantic_v2.0 branch from 1aea6f6 to f7aed3d Compare January 10, 2024 17:36

Remove deep_update

61c4089

adonath marked this pull request as ready for review January 10, 2024 17:47

adonath added 2 commits January 10, 2024 14:36

Fix remaining test fails

09e2611

Fix path serialization

751de19

bkhelifi reviewed Jan 11, 2024

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

maxnoe reviewed Jan 11, 2024

View reviewed changes

Update setup.cfg

0afd6de

Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>

registerrier reviewed Jan 11, 2024

View reviewed changes

adonath and others added 3 commits January 11, 2024 16:19

Use Optional for BackgroundMethode definiton

35a374b

Clean up and adress review comments

ae99c19

fix nan_results

fccc663

Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>

adonath added 3 commits January 12, 2024 11:17

Require scalar quantities, except for time

c6d1a41

Introduce our own deep_update

9bae571

Update codemeta and conda env file to require pydantic>=2.5

0617f2c

Fix Mapping import

b55836d

adonath added the infrastructure label Jan 12, 2024

adonath added this to the 1.2 milestone Jan 12, 2024

registerrier reviewed Jan 15, 2024

View reviewed changes

adonath added 2 commits January 15, 2024 16:24

Move json encoders

94c8f06

Add back quantity json encoder

16d849d

adonath force-pushed the support_pydantic_v2.0 branch from 4369788 to 16d849d Compare January 16, 2024 19:37

registerrier approved these changes Jan 16, 2024

View reviewed changes

bkhelifi approved these changes Jan 17, 2024

View reviewed changes

adonath merged commit 1438552 into gammapy:main Jan 17, 2024
27 of 29 checks passed

This was referenced Jan 24, 2024

CI broken: pydantic v2.0 API changes #4630

Closed

Modify config definition in modeling_2D #5045

Merged

chaimain mentioned this pull request Jun 21, 2024

Follow usage of pydantic from Gammapy or pursue something different? chaimain/asgardpy#175

Open

Support pydantic v2.0 #4750

Support pydantic v2.0 #4750

Conversation

adonath commented Sep 15, 2023 • edited Loading

adonath commented Sep 15, 2023

adonath commented Sep 18, 2023

dioptre commented Sep 25, 2023

adonath commented Nov 3, 2023

adonath commented Jan 10, 2024

adonath commented Jan 10, 2024 • edited Loading

adonath commented Jan 11, 2024

maxnoe commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxnoe Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

registerrier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MRegeard commented Jan 12, 2024

bkhelifi commented Jan 12, 2024

adonath commented Jan 12, 2024

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

adonath commented Jan 12, 2024

registerrier left a comment

Choose a reason for hiding this comment

adonath commented Jan 15, 2024

registerrier left a comment

Choose a reason for hiding this comment

adonath commented Jan 16, 2024

bkhelifi left a comment

Choose a reason for hiding this comment

adonath commented Jan 17, 2024

adonath commented Sep 15, 2023 •

edited

Loading

adonath commented Jan 10, 2024 •

edited

Loading

maxnoe Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading