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

0xDEC0DE · 2025-08-14T02:17:34Z

Make the MemoryBackend behave like the others, and stop removing keys from the initialization dict.

Fixes: Issue #273

zzzeek

so I'm a little curious what the use case to add non-consumed keys to the argument dictionary is, but at the same time it's clear the other backends have explicit code to prevent them from mutating the given dictionary so this patch is mostly about getting explicit test coverage for that, so let's do that with explicit tests. thanks for the PR

tests/cache/test_memory_backend.py

zzzeek · 2025-08-14T13:45:48Z

dogpile/testing/fixtures.py

            backend = backend_cls(arguments)
+            # Fail the test if the parameters have been altered by the backend
+            # setup; the results in such cases should be considered suspect
+            if arguments != stashed:


I played with this a bit and in pytest it produces an ERROR rather than FAILURE. I think this would be better done in an explicit test with a few scenarios:

arguments that we know are consumed, remain in the dictionary

arguments that we know are not consumed, remain in the dictionary

no new keys are added to the dictionary

these tests would be added to _GenericBackendTestSuite.

I was proposing new tests be added here to _GenericBackendTestSuite, which I dont see in the PR here. These can be added to the PR, dont worry about gerrit

sqla-tester

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

sqla-tester · 2025-08-14T13:47:36Z

New Gerrit review created for change 759dc77: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084

0xDEC0DE · 2025-08-14T18:03:50Z

so I'm a little curious what the use case to add non-consumed keys to the argument dictionary is, but at the same time it's clear the other backends have explicit code to prevent them from mutating the given dictionary so this patch is mostly about getting explicit test coverage for that, so let's do that with explicit tests. thanks for the PR

The use case is pedestrian enough that I'm shocked it hasn't ever come up: I have a library that uses dogpile.cache, and cheerfully pass through any/all dogpile.cache configs that exist in my library's config object. If configured to use dogpile.cache.memory with a cache_dict arg defined, it quietly eats part of my library's config object, which is sub-wonderful.

I currently have a guard against this behavior in place, but it seems like mutating inputs for no discernible reason should be frowned upon, at a minimum.

As for the workflow going forward, I am mildly confused. A robot appears to have migrated this into Gerrit (I used to work on OpenStack! I love and miss Gerrit!) but all of the comments still appear to be over here, and I don't have rights to see the Jenkins results over there. Do I push changes here, and they sync, or do I pivot to git review, or... ?

zzzeek · 2025-08-15T14:04:17Z

right so it's complicated

short answer, leave the comments here, just work w/ the PR like nothing is going on.

jenkins and now gerrit are behind a password because we have swarm bots crawling them and crashing my server (they are both heavy java VMs and can't serve requests that fast without pushing the load to 35). I'm going to see if cloudflare or something can fix this

zzzeek · 2025-08-15T14:05:13Z

the suites that failed are:

tox -e pep484

and

tox -e pep8

for the latter one I'd install pre-commit which will do the checks before you commit

zzzeek · 2025-08-15T17:54:46Z

jenkins and gerrit should be open now

Make the `MemoryBackend` behave like the others, and stop removing keys from the initialization dict. Fixes: Issue sqlalchemy#273 Closes: sqlalchemy#274 Pull-request: sqlalchemy#274 Pull-request-sha: 759dc77 Change-Id: Ifd3f0d88b18a73a0ce287e1a514a6268daf157c4

0xDEC0DE · 2025-08-16T06:44:08Z

jenkins and gerrit should be open now

Jenkins is accessible, but your Gerrit SSH endpoint seems unhappy (web UI works fine). I'll try pushing here to see if it works itself out.

Make the `MemoryBackend` behave like the others, and stop removing keys from the initialization dict. Drive-by: fix some PEP8 and PEP484 errors in unrelated files Fixes: Issue sqlalchemy#273 Closes: sqlalchemy#274 Pull-request: sqlalchemy#274 Pull-request-sha: 759dc77 Change-Id: Ifd3f0d88b18a73a0ce287e1a514a6268daf157c4

zzzeek · 2025-08-16T13:24:30Z

jenkins and gerrit should be open now

Jenkins is accessible, but your Gerrit SSH endpoint seems unhappy (web UI works fine). I'll try pushing here to see if it works itself out.

ive moved the ssh endpoint to ssh.gerrit.sqlalchemy.org

sqla-tester

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

sqla-tester

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

zzzeek · 2025-08-16T13:41:29Z

sorry for the troubles, we had to put gerrit behind cloudflare because bots were bringing the server down, so now we're juts hitting all the problems w that

sqla-tester · 2025-08-17T16:11:01Z

Michael Bayer (zzzeek) wrote:

when flake8 and friends have updates, things break. we could of course pin the versions but then we'd never upgrade it, so ill push fixes for whatever it is and you can rebase.

not sure who knows/doesnt but gerrit /jenkins should be fully accessible now, the worst you should get is a cloudflare clickthrough page maybe but probably/hopefully not.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084

sqla-tester

Michael Bayer (zzzeek) wrote:

lets:

rebase on main, I've fixed pep8/pep484
revert the fixes for pep8/pep484 which are addressed by main
make the remaining changes from my gh review in the test suite (dont cause setup errors, add dedicated all-backend test for dictionary not mutated)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084

dogpile/cache/backends/memcached.py

dogpile/cache/region.py

dogpile/testing/fixtures.py

sqla-tester · 2025-08-17T16:28:08Z

Michael Bayer (zzzeek) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084

Make the `MemoryBackend` behave like the others, and stop removing keys from the initialization dict. Fixes: Issue sqlalchemy#273 Closes: sqlalchemy#274 Pull-request: sqlalchemy#274 Pull-request-sha: 759dc77 Change-Id: Ifd3f0d88b18a73a0ce287e1a514a6268daf157c4

sqla-tester

Nicolas Simonds (0xDEC0DE) wrote:

The Gerrit SSH server claims I don't have write permission to push changes, but it's been a minute since I've used the Gerrit workflow, and I've likely forgotten the correct steps. I just shoved the changes into Github, since you appear to have the needed automation to sync things up, and that appears to work.

Sorry for all the noise and drama, I just wanted to give you a simple little fix! 😄

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084

dogpile/cache/backends/memcached.py (line 21): Done
dogpile/cache/region.py (line 820): Done
dogpile/testing/fixtures.py (line 39): Done

zzzeek · 2025-08-17T20:31:13Z

right you dont have gerrit write perms, it's a closed server. we dont make people learn gerrit we just synchronize their commits from their PR to our own gerrit server. i mean if you were contributing a lot, we could set you up with gerrit access, but it's just this one PR for now.

sqla-tester

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

sqla-tester · 2025-08-17T20:31:27Z

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

CaselIT · 2025-08-21T19:04:12Z

this seems now a subset of #270

zzzeek · 2025-08-22T20:04:08Z

OK, still waiting for generic tests so if #270 can add this, will transfer this to that one

0xDEC0DE · 2025-08-24T17:13:20Z

This now looks to be superseded by #270 and #275, which both solve this problem as a side-effect of how they are doing things. Closing this one rather than making things confusing.

zzzeek · 2025-08-30T13:14:35Z

I want this general fix in so I'm going to finish it up now

zzzeek · 2025-08-30T13:14:47Z

(another win for gerrit I still have your code! :) )

sqla-tester · 2025-08-30T14:24:08Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6084 has been merged. Congratulations! :)

Make the `MemoryBackend` behave like the others, and stop removing keys from the initialization dict. Fixes: #273 Closes: #274 Pull-request: #274 Pull-request-sha: b893879 Change-Id: Ifd3f0d88b18a73a0ce287e1a514a6268daf157c4

0xDEC0DE force-pushed the issue/273 branch from cad4e5c to 759dc77 Compare August 14, 2025 02:27

zzzeek requested changes Aug 14, 2025

View reviewed changes

zzzeek requested a review from sqla-tester August 14, 2025 13:47

sqla-tester reviewed Aug 14, 2025

View reviewed changes

0xDEC0DE force-pushed the issue/273 branch from 759dc77 to 9a47bdb Compare August 16, 2025 06:36

0xDEC0DE force-pushed the issue/273 branch from 9a47bdb to d3128bf Compare August 16, 2025 07:00

zzzeek requested a review from sqla-tester August 16, 2025 13:24

sqla-tester reviewed Aug 16, 2025

View reviewed changes

zzzeek requested a review from sqla-tester August 16, 2025 13:28

sqla-tester reviewed Aug 16, 2025

View reviewed changes

zzzeek self-requested a review August 16, 2025 13:30

sqla-tester reviewed Aug 17, 2025

View reviewed changes

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

dogpile/cache/region.py Outdated Show resolved Hide resolved

dogpile/testing/fixtures.py Outdated Show resolved Hide resolved

0xDEC0DE force-pushed the issue/273 branch from d3128bf to b893879 Compare August 17, 2025 19:41

sqla-tester reviewed Aug 17, 2025

View reviewed changes

zzzeek requested a review from sqla-tester August 17, 2025 20:31

sqla-tester reviewed Aug 17, 2025

View reviewed changes

zzzeek mentioned this pull request Aug 18, 2025

Feature typeddict #270

Open

0xDEC0DE closed this Aug 24, 2025

0xDEC0DE deleted the issue/273 branch August 24, 2025 17:13

0xDEC0DE mentioned this pull request Aug 24, 2025

Feature typeddict alt #275

Closed

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

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

Uh oh!

Conversation

0xDEC0DE commented Aug 14, 2025

Uh oh!

zzzeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zzzeek Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

zzzeek Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Aug 14, 2025

Uh oh!

0xDEC0DE commented Aug 14, 2025

Uh oh!

zzzeek commented Aug 15, 2025

Uh oh!

zzzeek commented Aug 15, 2025

Uh oh!

zzzeek commented Aug 15, 2025

Uh oh!

0xDEC0DE commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zzzeek commented Aug 16, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

zzzeek commented Aug 16, 2025

Uh oh!

sqla-tester commented Aug 17, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sqla-tester commented Aug 17, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

zzzeek commented Aug 17, 2025

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Aug 17, 2025

Uh oh!

CaselIT commented Aug 21, 2025

Uh oh!

zzzeek commented Aug 22, 2025

Uh oh!

0xDEC0DE commented Aug 24, 2025

Uh oh!

zzzeek commented Aug 30, 2025

Uh oh!

zzzeek commented Aug 30, 2025

Uh oh!

sqla-tester commented Aug 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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

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

0xDEC0DE commented Aug 16, 2025 •

edited

Loading