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

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

Closed
gilesknap opened this issue May 13, 2024 · 5 comments
Closed
Assignees

Comments

@gilesknap
Copy link
Member

gilesknap commented May 13, 2024

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(
@gilesknap gilesknap changed the title Pydantic 2.71 causes IOC instance generation failures Pydantic 2.7.1 causes IOC instance generation failures May 13, 2024
@gilesknap gilesknap changed the title Pydantic 2.7.1 causes IOC instance generation failures Pydantic 2.7.1 causes IOC instance generation warnings (and test failures) May 13, 2024
@marcelldls
Copy link
Contributor

Rather appears like a possible bug? related to type Literal pydantic/pydantic#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 pydantic/pydantic#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
Copy link
Member Author

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
Copy link
Contributor

coretl commented May 23, 2024

Maybe pvi too? @GDYendell

@GDYendell
Copy link
Member

GDYendell commented May 23, 2024

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
Copy link
Member Author

Thanks @marcelldls your suggestion fixed the issue and the dependabot PR is merged.
#227

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

No branches or pull requests

4 participants