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

Raise an exception when Pydantic plugin is enabled on Pydantic <2.5.0 #160

Merged
merged 11 commits into from
May 14, 2024

Conversation

bossenti
Copy link
Contributor

Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@alexmojaki
Copy link
Contributor

I'm not sure what the minimum supported version is, but it's definitely less than 2.7.0.

@Kludex
Copy link
Member

Kludex commented May 10, 2024

I think it's 2.6.2 - need to check.

@bossenti
Copy link
Contributor Author

bossenti commented May 10, 2024

I can only say that it hasn't worked with 2.4.2 but with 2.7.0.
Best would be Samuels solution and adding a console statement, I just filed this PR so there is at least some information about it.
But I'm fine to close it if you don't think it's appropriate.

@Kludex
Copy link
Member

Kludex commented May 13, 2024

2.5.0 is the minimum version.

@Kludex Kludex self-assigned this May 13, 2024
@Kludex Kludex requested a review from alexmojaki May 13, 2024 10:56
@Kludex Kludex force-pushed the docs-pydantic-note branch from 2107534 to 997dfa8 Compare May 13, 2024 11:13
@Kludex Kludex changed the title docs: add note about minimal pydantic version Raise on exception when Pydantic plugin is enabled on Pydantic <2.5.0 May 13, 2024
import pydantic

# setuptools is a dependency of opentelemetry-instrumentation, we use it instead of adding "packaging" as a dependency.
from setuptools._vendor.packaging.version import Version
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like it if we had a util function which tried to import things from packaging and then fell back to setuptools._vendor.packaging, so that we use the proper module where possible. instrument_psycopg could then also use that and we could remove the packaging dep from the extra.

@@ -292,61 +291,68 @@ def get_schema_name(schema: CoreSchema) -> str:
class LogfirePydanticPlugin:
"""Implements a new API for pydantic plugins.

Patches pydantic to accept this new API shape.
Patches Pydantic to accept this new API shape.

Set the `LOGFIRE_DISABLE_PYDANTIC_PLUGIN` environment variable to `true` to disable the plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set the `LOGFIRE_DISABLE_PYDANTIC_PLUGIN` environment variable to `true` to disable the plugin.
Set the `LOGFIRE_DISABLE_PYDANTIC_PLUGIN` or `PYDANTIC_DISABLE_PLUGINS` environment variable to `true` to disable the plugin.

Copy link
Member

Choose a reason for hiding this comment

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

The reason I didn't do this, is because I'd have to explain that PYDANTIC_DISABLE_PLUGINS disables all the plugins. I don't think it's necessary to do it here.

@Kludex Kludex requested a review from alexmojaki May 13, 2024 11:50
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
@Kludex Kludex enabled auto-merge (squash) May 13, 2024 22:10
auto-merge was automatically disabled May 13, 2024 22:11

Base branch was modified

@Kludex Kludex requested a review from alexmojaki May 14, 2024 09:39
@alexmojaki alexmojaki changed the title Raise on exception when Pydantic plugin is enabled on Pydantic <2.5.0 Raise an exception when Pydantic plugin is enabled on Pydantic <2.5.0 May 14, 2024
@Kludex Kludex enabled auto-merge (squash) May 14, 2024 09:53
@Kludex Kludex merged commit 4e94b68 into pydantic:main May 14, 2024
10 checks passed
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