Skip to content
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

memoization: with named parameter but positional argument, memoization ignore={} doesn't apply #240

Open
tnielens opened this issue Dec 6, 2021 · 4 comments

Comments

@tnielens
Copy link

tnielens commented Dec 6, 2021

@cache.memoize(ignore={'session'})
def get(entity_id: str, session):
  session.get(entity_id, timeout=..., ...)

session = db.Session(...)
# diskcache tries to serialize session and fails
get(1, session)

@cache.memoize(ignore={'session', 1}) works fine. But it's a little error-prone.

diskcache version: 5.3.1

@grantjenks
Copy link
Owner

By design 🤷‍♂️

Could make “get” use keyword-only arguments (https://www.python.org/dev/peps/pep-3102/) to prevent the footgun.

@tnielens
Copy link
Author

tnielens commented Dec 6, 2021

I expected the ignore parameter to work without specifying my parameter position. A bit of python doc on this might be helpful for users (I didn't find any). Maybe interesting to take into consideration: I'm changing one of my project from joblib to diskcache for memoization. Joblib implements the alternative approach and lets the user only specify the parameter name.

In any case, diskcache is really nice and I'm happy with the switch 👍!

@grantjenks
Copy link
Owner

Better docs would probably be good. If you want to add a sentence or two to the memoize docstrings then I think that would be a good place. Pull request welcome 👍

@grantjenks
Copy link
Owner

I used this in anger and thought it could be done better. It would be nice if in the case given at the top, “session” would automatically work for either the keyword argument or positional argument. I get the ergonomics now.

Here’s the code in joblib: https://github.com/joblib/joblib/blob/0730b779fc9713d2deff66e1685d84e3f8530c60/joblib/func_inspect.py#L197 . Little complex but maybe that’s the best here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants