fix: Remove pydantic constraint <2.10.0 and update timedelta validator, serializer type hints #757
Conversation
Seems like new 2.10.0 version made some changes that made our type hints cause errors. On the other hand, it seems our type hints were too generic. Making them more specific fixes the issue.
github-actions
bot
added
the
t-tooling
src/crawlee/_utils/models.py
Outdated
| def _timedelta_from_ms(value: float | timedelta | Any | None, handler: Callable[[Any], Any]) -> Any: | ||
| def _timedelta_from_ms( | ||
| value: float | timedelta | Any | None, handler: Callable[[Any], timedelta | None] | ||
| ) -> timedelta | None: |
There was a problem hiding this comment.
Same as _timedelta_to_ms comment
| @@ -9,7 +9,7 @@ | |||
| """Utility types for Pydantic models.""" | |||
|
|
|||
|
|
|||
| def _timedelta_to_ms(td: timedelta | None) -> Any: | |||
| def _timedelta_to_ms(td: timedelta | None) -> float | None: | |||
There was a problem hiding this comment.
We could be more strict and require float, but I have a feeling, that None could be used for something connected with defaults somewhere in code???
There was a problem hiding this comment.
I guess we can consider using only floats.
However, in StatisticsState and StatisticsPersistedState, we have fields like this:
request_min_duration: Annotated[timedelta_ms | None, Field(alias='requestMinDurationMillis')] = NoneMake sure it won't break them.
There was a problem hiding this comment.
I tried adding some tests (but not sure whether I should keep them) and I think it works as expected. I am not really so sure why the previous implementation allowed any in so many places in this file, so I am little afraid there is some non obvious edge case I could not think of...
There was a problem hiding this comment.
Looks good. Thanks. Let's wait for what @janbuchar thinks about that.
There was a problem hiding this comment.
Is there a good reason not to support | None in the serializer? Does it break pydantic >2.10? If not, I'd keep it, just in case - it looks like pydantic 2.10 doesn't call the serializer for None values, but that might not be the case for older versions which we also claim to support.
There was a problem hiding this comment.
Well that can also break pydantic. See new commit that allows | None, but has to use Union due to known issues that Pydantic can have with from __future__ import annotations and | symbol not being available for older Python versions in type hints...
There was a problem hiding this comment.
Are you sure that the pipe syntax will work with Pydantic 3.9, for example?
Add tests for Models using this type.
github-actions
bot
added
the
tested
vdusek
modified the milestones:
103rd sprint - Tooling team,
104th sprint - Tooling team
| (3.5, timedelta(microseconds=3500), 4), | ||
| (3.99, timedelta(microseconds=3990), 4), | ||
| (None, None, None), | ||
| (float('inf'), timedelta(days=999999999, seconds=3600 * 24 - 1, microseconds=999999), float('inf')), |
There was a problem hiding this comment.
I considered 3600 * 24 - 1 to be more readable than 86399. Since it is just a test, I avoided defining SECONDS_IN_HOUR and HOURS_IN_DAYS ....
There was a problem hiding this comment.
Thanks for avoiding that 🙂 I usually do timedelta(days=3, hours=8).total_seconds() instead.
Edit pyproject toml to ensure that the broken versions are forbidden. Use | insted of union and bring back from future import annotations
Versions 2.6.0-2.8.0 are failing our unit tests
…r, serializer type hints (apify#757) Pydantic version 2.10.0 made some changes that broke our code. This was fixed in 2.10.3 Also our type hints were too generic in models.py. This change makes them more specific. Older versions of Pydantic 2.6.0-2.8.0 caused errors in our codebase and so minimal version of Pydantic was bumped to 2.8.1.
Seems like new 2.10.0 version made some changes that made our type hints cause errors. On the other hand, it seems our type hints were too generic. Making them more specific fixes the issue in new pydantic versions.