fix: Adding a TypeAdapter cache to fix a performance regression with … #212
Conversation
| A TypeAdapter for the given type. | ||
| """ | ||
| # Use the string representation of the type as a cache key | ||
| type_key = str(field_type) |
There was a problem hiding this comment.
Did you try id as an alternative to str as well? If types aren't too redundant, it might be a bit faster.
There was a problem hiding this comment.
It was indeed an improvement! I had some concerns about how those ids would resolve/collide but after doing a bit more investigation/testing looks like this is the way to go. Thanks!
| # Verify that the operation completed within a reasonable time | ||
| assert ( | ||
| elapsed_time < 10 | ||
| ), f"Operation took {elapsed_time:.2f} seconds, which exceeds the 10 second threshold" |
There was a problem hiding this comment.
How close are we to this threshold?
There was a problem hiding this comment.
It's going to depend on the host which is part of why I've relegated it to "benchmark", but I'm running it in about 4 seconds currently
| sync = "pip install -r requirements-testing.txt" | ||
| test = "pytest --cov-config pyproject.toml {args:test}" | ||
| test = "pytest test/ --cov-config pyproject.toml --ignore=test/openjd/model/benchmark {args}" | ||
| benchmark = "pytest test/openjd/model/benchmark --no-cov {args}" |
There was a problem hiding this comment.
Should we be running these in GitHub actions to prevent regression before merge?
There was a problem hiding this comment.
I was looking at that - I'd like to, but it looks like the commands run live in a shared config currently. If there's a simple way to add that I'm happy to do it though!
…larger templates. Signed-off-by: Brian Axelson <[email protected]>
|
OpenJobDescription#212) fix: Adding a TypeAdapter cache to fix a performance regression with larger templates. Signed-off-by: Brian Axelson <[email protected]>
4 participants
What was the problem/requirement? (What/Why)
Recent changes to type validation have created a performance bottleneck with larger templates in pydantic's TypeAdapter.
What was the solution? (How)
What is the impact of this change?
~87% performance improvement when testing with larger templates
How was this change tested?
Was this change documented?
Yes
Is this a breaking change?
No
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.