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

Remove pydantic-yaml as dependency #110

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Remove pydantic-yaml as dependency #110

merged 1 commit into from
Jan 30, 2025

Conversation

agoscinski
Copy link
Collaborator

The amount of functionality we use of this library is extremely minimal. We replace it with the dependency ruamel.yaml that parses the yaml file into a python object. This is basically the code we use from the dependency https://github.com/NowanIlfideme/pydantic-yaml/blob/130569dabbe9843771bc6f7d52830e061238d61b/src/pydantic_yaml/_internals/v2.py#L217-L242

Furthermore, the library still uses pydantic API v1 for validating the pydantic model which is deprecated so we can replace it with pydantic v2. In addition this change gives us more flexibility as it separates the parsing of the yaml file and the validation through pydantic.

To have a similar utility function that can be used in the docstring tests we implement validate_yaml_content.

@agoscinski agoscinski marked this pull request as ready for review January 27, 2025 15:20
@agoscinski agoscinski requested a review from DropD January 27, 2025 15:20
Comment on lines 736 to 739
from io import StringIO

from pydantic import TypeAdapter
from ruamel.yaml import YAML
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why importing locally to the function? This will be executed each time the function is called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I kind of imitated the style before. I think @DropD introduced this in a PR. My understanding is that this is only loaded if the function is called (and then cached so rerunning is not an issue). This makes it faster to do a top level import (import sirocco) since not all dependencies that are used in the library are immediately loaded. I am not sure about a guideline here. It makes sense to do it for ruamel since it will be only needed in a few places and importing it can take some time, but if for TypeAdapterand StringIO I am not sure if makes such a difference. Maybe @DropD has a comment or correction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it hides in the function what the module depends on. Curious about @DropD thoughts on that.

@leclairm
Copy link
Contributor

leclairm commented Jan 28, 2025

Other than my small comment I'm fine with the PR. I thought pydantic-yaml was developped by the same people as pydantic but apparently not. Hence the lag in using v2 I guess.

Also may I ask why ruamel.yaml?

@agoscinski
Copy link
Collaborator Author

Also may I ask why ruamel.yaml?

I just used the library that pydantic-yaml was using. We have now the flexibility to change it but I did not investigate the differences. Since it is used with particular options YAML(typ="safe", pure=True). One could definitely improve this in the future, for example, pure=True uses the default standard Python implementation to load the yaml which is more compatible over systems over using the C-extension but is also slower. I would investigate this in the future when we have a more robust CI (including CI on alps) that could catch potential errors when switching to less compatible implementation. I should make an issue for this.

@leclairm
Copy link
Contributor

I should make an issue for this

Thanks for the explanation but don't bother, this is really low priority.

Copy link
Collaborator

@GeigerJ2 GeigerJ2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@agoscinski
Copy link
Collaborator Author

We decided to move the imports to the header for better overview, the advantage of not importing ruamel when using the library only with the python API is not really meaningful for us. So to have a better overview is more important

The amount of functionality we use of this library is extremly minimal.
We replace it with the dependency `ruamel.yaml` that parses the yaml
file into a python object. Furthemorer, the library still uses pydantic
API v1 for validating the pydantic model which is deprecated so we can
replace it with pydantic v2. In addition this change gives us more
flexibility as it separates the parsing of the yaml file and the
validation through pydantic.

To have a similar utility function that can be used in the docstring
tests we implement `validate_yaml_content`.
@agoscinski agoscinski merged commit 4cd4c0e into main Jan 30, 2025
6 checks passed
@agoscinski agoscinski deleted the rm-pydantic-yaml branch January 30, 2025 11:04
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.

3 participants