Feature typeddict #270

jvanasco · 2025-08-05T02:01:39Z

This is the idea for using typedicts

I also updated some typing and docstrings

Change-Id: I30619425e1e1e4ca43286be35a8c4a85658a8acb

include missing NO_VALUE on docstrings Change-Id: I79c4657d5ea8778e2932bae3e81725af27bbef85

Change-Id: I8cd5d4fbf7eabf03e48f5434e6c50dacd07dfd3c

Change-Id: Ib1c0a38f072676f33254a91312e7237c129850f2

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 1821cb9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-08-06T16:44:02Z

New Gerrit review created for change 1821cb9: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6081

CaselIT

great, left just a suggestion to dry up a bit the definition

dogpile/cache/backends/redis.py

* applied typing to other backends * standardized all backends to operate on a copy of the Dict passed in Some backends did this, others did not. Change-Id: I90084c6d0ad23d7bb9988664c2cef1a739fa3ff1

Change-Id: I4b912cfdb7fdae970bd95d418e059bb9965b5832

jvanasco · 2025-08-11T16:31:34Z

Ready for another review.

General Concerns:

Redis Backend:

socket_keepalive_options Redis has this typed as: https://redis.readthedocs.io/en/latest/connections.html

socket_keepalive_options (Optional[Mapping[int, Union[int, bytes]]])
but test_redis_backend has

"socket_keepalive_options": {4, 10.0}

test_decorator.py

test_sha1_key_mangler_bytes_py3k uses bytes
test_sha1_key_mangler_bytes_py2k should this exist anymore?

dogpile/cache/util.py

not really sure how to type the callable args and returns. i left them generic.

Backends
There was a lot of cruft from decades of dogpile and changing styles. i standardized everything to the following:

_arguments = arguments.copy() - some backends still .pop() a value off the arguments dict., others use .get(). iirc this was a legacy from operating on the dict then passing it on to a third party library constructor. i did not change pop vs get, but everything now operates on a copy. I also cast the copy to Dict, so items can be popped (that emits errors on typeddicts). I could easily do that.
to deal with superclasses and typing, I created another .copy() and changed type with cast. I'm not particularly happy with this, but it works.

I wasn't too happy with this approach, but I there is no real performance/memory hit (it happens on backend startup), and most new backends will copy/edit an existing backend - so I wanted to set a unified style to reinforce.

This definitely needs review and feedback, but IMHO these upgrades to typing are helping find issues.

CaselIT

left a suggestion, looks great overall to me

regarding get vs pop, in my opinion we could replace all of the with gets, but let's wait for @zzzeek opinion

CaselIT · 2025-08-18T09:01:04Z

dogpile/cache/backends/file.py


-    def __init__(self, arguments):
+    def __init__(self, arguments: DBMBackendArguments):
+        _arguments = cast(Dict, arguments.copy())


since this backend doesn't modify the dicy, maybe we could avoid this?

I mean it may make sense for backends that use pop, but for the other maybe not? this applies also to the other cases

I opted to standardize how all the backends work, because people will use them as reference when implementing a new one.

CaselIT · 2025-08-18T09:03:14Z

dogpile/cache/backends/memcached.py

+        self.username = _arguments.get("username", None)
+        self.password = _arguments.get("password", None)
+        self.tls_context = _arguments.get("tls_context", None)
+        _arguments_super = cast(GenericMemcachedBackendArguments, _arguments)


this isn't great, also considering that this method doesn't modify the argument

I'm not thrilled with it either, this was purely to appease the mypy.

CaselIT · 2025-08-18T09:05:21Z

dogpile/cache/backends/memcached.py

     .. versionadded:: 1.1.5

-    :param dead_timeout: Time in seconds before attempting to add a node
+    :param hashclient_dead_timeout: Time in seconds before attempting to add a node


would this require a versionchanged note in the docstring and/or backward compat property what warns on access?

No. This is just correcting a docstring error. The docstring references dead_timeout, the code uses hashclient_dead_timeout see main:

https://github.com/sqlalchemy/dogpile.cache/blob/main/dogpile/cache/backends/memcached.py#L610

oh ok, the docs is of the dictionary not of the class attributes. got it.

jvanasco · 2025-08-18T14:35:42Z

regarding get vs pop, in my opinion we could replace all of the with gets,

I'd prefer that. IIRC, the usage of pop is a legacy behavior. these constructors once popped off the vars they didn't need, then passed the mutated dicts onto other functions (such as the redis or memcached client contructors).

If that is not the case anymore, I'd love to standardize everything to whatever style you and mike prefer. that would drop the need for .copy() of args as well. i'd just like to see every backend implemented with the same coding conventions.

CaselIT · 2025-08-18T15:40:21Z

I would go for get, but let's wait for mike here

zzzeek · 2025-08-18T21:50:35Z

regarding get vs pop, in my opinion we could replace all of the with gets,

I'd prefer that. IIRC, the usage of pop is a legacy behavior. these constructors once popped off the vars they didn't need, then passed the mutated dicts onto other functions (such as the redis or memcached client contructors).

If that is not the case anymore, I'd love to standardize everything to whatever style you and mike prefer. that would drop the need for .copy() of args as well. i'd just like to see every backend implemented with the same coding conventions.

OK, yes we can use get, but we are in a collision here. Please take a look at github.com//pull/274 which is adding tests for this functionality and also fixing the memory backend. I've asked for some changes and I'm not sure the contributor understands what I'm asking for (I want a test in test_region for all backends illustrating that backends dont mutate the parameter dictionaries).

jvanasco · 2025-08-18T21:54:52Z

Let me see what I can pull together in the next 30 minutes.

fixed valkey default for lock_sleep Change-Id: I03c80c105be4ec3b280d57f340b050b16c4d937a

jvanasco · 2025-08-18T23:22:10Z

I added some backend tests as BackendArgsAntiMutationTest. I'm hoping the redis/valkey clusters pass in the cloud; I can't run them locally due to an instant fail if they cluster servers are not available.

while doing this, I noticed there are no tests for the redis & valkey cluster backends.

I did something weird, as I wasn't sure how to pull off elegantly. when a backend is tested, I add it to BackendArgsAntiMutationTest.backends_tested. on the class teardown, I iterate over the plugin loader to ensure this test has run on every backend that is registered. This will cause a fatal Error on the final test if a new backend has been created, but is not added to this test class. You may think of a better way to implement this.

CaselIT

great work, just an additional suggestion, but it's fine as is too

dogpile/cache/backends/memcached.py

Change-Id: Ib7bf3ebf825c0823e70b29b0ef5ee5f21010616a

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 0680d81 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-08-19T17:45:41Z

Failed to create a gerrit review, git squash against branch 'main' failed

CaselIT · 2025-08-21T19:45:27Z

@jvanasco could you merge with the main branch?

Change-Id: I4b00ec1e4faa45b204bc214099d0180c46b08145

jvanasco · 2025-08-21T21:06:00Z

@CaselIT Done, but there is a merge commit

CaselIT · 2025-08-22T07:24:38Z

it's not an issue since gerrit will squash everything

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 9eb6fc3 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-08-22T07:24:44Z

Patchset 9eb6fc3 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6081

zzzeek · 2025-08-22T20:03:44Z

im dying for either of this #270 or #274 to please put a test that is across all backends (meaning, in dogpile/cache/testing/fixtures) to assert that:

keys that are consumed by backends remain in the argument dictionary afterwards
keys that are unknown to the backend don't cause probelms and also remain in the argument dictionary

jvanasco · 2025-08-22T21:09:06Z

im dying for either of this #270 or #274 to please put a test that is across all backends (meaning, in dogpile/cache/testing/fixtures) to assert that:

keys that are consumed by backends remain in the argument dictionary afterwards

keys that are unknown to the backend don't cause probelms and also remain in the argument dictionary

I implemented #1 in test_region as class BackendArgsAntiMutationTest; #2 would just be tossing in some random values. I can certainly shift it to a class-based system that is available to third party plugins who use those fixtures.

jvanasco · 2025-08-23T00:11:00Z

I switched the backend test from test_region to GenericBackendFixture as requested, it's in #275 as separate PR for easier comparison.

I do want to note there is no exiting test coverage (in general) for redis & valkey cluster backends. that is outside the scope of this though, as it will likely require a CI setup.

Change-Id: I90beea7da33de522de95e3e901410a5966aea024

Change-Id: I6a9b9f45ecb9c8acf6e030aacfb4ec2f28b0b689

jvanasco · 2025-09-11T00:51:45Z

This has been updated against main. The previously requested tests were removed as they are now unnecessary.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 8711197 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2025-09-11T07:21:38Z

Patchset 8711197 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6081

jvanasco added 3 commits August 4, 2025 21:54

some typing updates

134f44d

Change-Id: I30619425e1e1e4ca43286be35a8c4a85658a8acb

extend proxy docs with typing

2d81ec1

include missing NO_VALUE on docstrings Change-Id: I79c4657d5ea8778e2932bae3e81725af27bbef85

more typing

ca15a91

Change-Id: I8cd5d4fbf7eabf03e48f5434e6c50dacd07dfd3c

jvanasco marked this pull request as draft August 5, 2025 04:29

remove CacheMutex as base class

1821cb9

Change-Id: Ib1c0a38f072676f33254a91312e7237c129850f2

jvanasco marked this pull request as ready for review August 5, 2025 14:04

zzzeek requested review from CaselIT and sqla-tester August 6, 2025 16:43

sqla-tester reviewed Aug 6, 2025

View reviewed changes

CaselIT reviewed Aug 6, 2025

View reviewed changes

dogpile/cache/backends/redis.py Outdated Show resolved Hide resolved

* swtich TypedDicts to use total=False instead of NotRequired

23e69ed

* applied typing to other backends * standardized all backends to operate on a copy of the Dict passed in Some backends did this, others did not. Change-Id: I90084c6d0ad23d7bb9988664c2cef1a739fa3ff1

jvanasco marked this pull request as draft August 11, 2025 15:19

more standardization and typing

9e4f016

Change-Id: I4b912cfdb7fdae970bd95d418e059bb9965b5832

jvanasco marked this pull request as ready for review August 11, 2025 16:21

CaselIT reviewed Aug 18, 2025

View reviewed changes

tests for guard against region args mutation

f3e42ff

fixed valkey default for lock_sleep Change-Id: I03c80c105be4ec3b280d57f340b050b16c4d937a

CaselIT approved these changes Aug 19, 2025

View reviewed changes

dogpile/cache/backends/memcached.py Outdated Show resolved Hide resolved

drop unnecessary cast

0680d81

Change-Id: Ib7bf3ebf825c0823e70b29b0ef5ee5f21010616a

CaselIT requested a review from sqla-tester August 19, 2025 17:45

sqla-tester reviewed Aug 19, 2025

View reviewed changes

CaselIT mentioned this pull request Aug 21, 2025

fix: make dogpile.cache.memory preserve its init parameters #274

Closed

Merge branch 'main' into feature-typeddict

9eb6fc3

Change-Id: I4b00ec1e4faa45b204bc214099d0180c46b08145

CaselIT requested a review from sqla-tester August 22, 2025 07:24

sqla-tester reviewed Aug 22, 2025

View reviewed changes

jvanasco mentioned this pull request Aug 23, 2025

Feature typeddict alt #275

Closed

0xDEC0DE mentioned this pull request Aug 24, 2025

dogpile.cache.memory is curiously destructive of its inputs #273

Closed

jvanasco added 2 commits September 10, 2025 20:44

Merge branch 'main' into feature-typeddict

fc67637

Change-Id: I90beea7da33de522de95e3e901410a5966aea024

remove BackendArgsAntiMutationTest in favor of merged commit

8711197

Change-Id: I6a9b9f45ecb9c8acf6e030aacfb4ec2f28b0b689

CaselIT requested a review from sqla-tester September 11, 2025 07:21

sqla-tester reviewed Sep 11, 2025

View reviewed changes

Feature typeddict #270

Are you sure you want to change the base?

Feature typeddict #270

Uh oh!

Conversation

jvanasco commented Aug 5, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Aug 6, 2025

Uh oh!

CaselIT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jvanasco commented Aug 11, 2025

Uh oh!

CaselIT left a comment

Choose a reason for hiding this comment

Uh oh!

CaselIT Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

jvanasco Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

CaselIT Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

jvanasco Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

CaselIT Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

jvanasco Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

CaselIT Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

jvanasco commented Aug 18, 2025

Uh oh!

CaselIT commented Aug 18, 2025

Uh oh!

zzzeek commented Aug 18, 2025

Uh oh!

jvanasco commented Aug 18, 2025

Uh oh!

jvanasco commented Aug 18, 2025

Uh oh!

CaselIT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Aug 19, 2025

Uh oh!

CaselIT commented Aug 21, 2025

Uh oh!

jvanasco commented Aug 21, 2025

Uh oh!

CaselIT commented Aug 22, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Aug 22, 2025

Uh oh!

zzzeek commented Aug 22, 2025

Uh oh!

jvanasco commented Aug 22, 2025

Uh oh!

jvanasco commented Aug 23, 2025

Uh oh!

jvanasco commented Sep 11, 2025

Uh oh!