epics-containers / ibek

IOC Builder for EPICS and Kubernetes
https://epics-containers.github.io/ibek
Apache License 2.0
10 stars 4 forks source link

Pydantic 2.7.1 causes IOC instance generation warnings (and test failures) #217

Closed gilesknap closed 1 month ago

gilesknap commented 1 month ago

It looks like the latest Pydantic has changed the way in which it validates enums.

If I upgrade to Pydantic 2.7.1 and run tests/generate_samples.sh the first line that tries to generate IOC runtime assets gets multiple errors of the form below.

I'm assigning this issue to @marcelldls because it might be a good introduction to Pydantic and ibek's use of it.

Also see #210 where dependabot tries out this version of Pydantic/

+ echo making ioc based on quadem support yaml
making ioc based on quadem support yaml
++ pwd
+ EPICS_ROOT=/workspaces/ibek/tests/samples/epics
+ ibek runtime generate iocs/quadem.ibek.ioc.yaml support/ADCore.ibek.support.yaml support/quadem.ibek.support.yaml
/venv/lib/python3.10/site-packages/pydantic/main.py:347: UserWarning: Pydantic serializer warnings:
  Expected `enum` but got `str` - serialized value may not be as expected
  return self.__pydantic_serializer__.to_python(
/venv/lib/python3.10/site-packages/pydantic/main.py:347: UserWarning: Pydantic serializer warnings:
  Expected `enum` but got `str` - serialized value may not be as expected
  Expected `enum` but got `str` - serialized value may not be as expected
  return self.__pydantic_serializer__.to_python(
marcelldls commented 1 month ago

Rather appears like a possible bug? related to type Literal https://github.com/pydantic/pydantic/issues/9402 - In test_ibek_schema trace for example you will find:

                    'type': {
                        'const': 'bool',
                        'default': 'bool',
  -                     'enum': [
  ?                       ---   ^
  +                     'title': 'Type',
  ?                      ++++    ^^^^^^^
  -                         'bool',
  -                     ],
  -                     'title': 'Type',
  -                     'type': 'string',

So each entity class type field is serialized as an enum.

I believe this is then why we get the Expected 'enum' but got 'str' - serialized value may not be as expected. Applying use_enum_values=True to BaseSettings then seems to solve this https://docs.pydantic.dev/latest/api/config/#pydantic.config.ConfigDict.use_enum_values

According to https://github.com/pydantic/pydantic/issues/6565 seems use_enum_values may be deprecated. However maybe this is an option until the proposed type hint for const exists which looks like it would preserve the current behavior?

gilesknap commented 1 month ago

Thanks @marcelldls that seems like a decent fix.

Do you know if this affects the Definition arg type enum? I assume there is a test for this arg type already so hopefully that would have been picked up.

@coretl do any of our other pragmatic projects use the type literal approach. Maybe if so they are affected too?

coretl commented 1 month ago

Maybe pvi too? @GDYendell

GDYendell commented 1 month ago

I have updated pvi to use pydantic>2.7 and it just meant updating the schemas. The tests still passed.

Is it the case for ibek that it is just a schema change or does it actually not work?

pvi does use use_enum_values but only for very specific things - using text instead of numbers for string format enums in the yaml files (e.g. "string" instead of "4"). This could be removed pretty easily with some extra logic.

gilesknap commented 1 month ago

Thanks @marcelldls your suggestion fixed the issue and the dependabot PR is merged. https://github.com/epics-containers/ibek/pull/227