-
Notifications
You must be signed in to change notification settings - Fork 19.3k
fix(core): support for Python 3.14 #33461
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
Conversation
CodSpeed Performance ReportMerging #33461 will degrade performances by 12.27%Comparing
|
Mode | Benchmark | BASE |
HEAD |
Change | |
---|---|---|---|---|---|
👁 | WallTime | test_import_time[ChatPromptTemplate] |
572.6 ms | 652.6 ms | -12.27% |
👁 | WallTime | test_import_time[LangChainTracer] |
412.3 ms | 461.9 ms | -10.73% |
👁 | WallTime | test_import_time[PydanticOutputParser] |
505.2 ms | 574.5 ms | -12.05% |
👁 | WallTime | test_import_time[RunnableLambda] |
473 ms | 534.9 ms | -11.57% |
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
The coroutine with the context. | ||
""" | ||
if asyncio_accepts_context(): | ||
if sys.version_info >= (3, 11): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature we get in py314 is asyncio.create_task(coro, **kwargs)
so we can't check the presence of context
.
There's very little chance that support for context in create_task
will ever be removed so I think we can just do a version check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to fix the implementation of the function: asyncio_accepts_context instead of not using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
||
loop = asyncio.get_event_loop() | ||
try: | ||
loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asyncio.get_event_loop
raises an exception in Python 3.14 if there's no running loop (was a warning in previous versions)
c05fe27
to
42fcb96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very reasonable to me left a few nit comments
The coroutine with the context. | ||
""" | ||
if asyncio_accepts_context(): | ||
if sys.version_info >= (3, 11): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to fix the implementation of the function: asyncio_accepts_context instead of not using it?
f_or_c: Literal["F", "C"] | ||
forecast: str | ||
|
||
_FORECAST_MODELS_TYPES |= type[ForecastV1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_FORECAST_MODELS_TYPES
-- which type is this? something looks off. If it's meant to be a set or a list could we use the dedicated APIs for adding an element instead of relying on the operator shorthand (|=
) to maximize for readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_FORECAST_MODELS_TYPES
is a typing.Union
of types. Used for type checking. I don’t think there is an API to do this. But there’s maybe a simpler way to write it. I’ll have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to simplify. PTAL.
|
||
|
||
@pytest.mark.skipif( | ||
sys.version_info >= (3, 14), reason="Pydantic v1 not supported with Python 3.14+" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could be helpful to paraphrase the reason a bit (wording is confusing w/ respect to whether this is pydantic 1 or pydantic 2's v1 namespace)
You could say The pydantic.v1 namespace is not supported with python 3.14+
or something like that (to make it clear that it's specifically v1 within pydantic 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
b845d61
to
29c5034
Compare
@eyurtsev in the end, I added the upgrade to pydantic 2.12 to this PR. |
asyncio.create_task
asyncio.get_event_loop()
raises an exception if there's no running loop