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

Steps towards more symbolic dict keys in memoize_on_first_arg, memoize_method #80

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

inducer
Copy link
Owner

@inducer inducer commented Apr 15, 2021

No description provided.

cache_dict_name = intern(
f"_memoize_dic_{function.__module__}{function.__name__}"
)
cache_dict_name = (memoize_on_first_arg, function)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if function is nested (i.e. not in the global namespace)? It should keep changing its hash/id in that case, right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, nested functions are a difficult case, mainly because it's hard to get stable identifiers for them. My understanding is that, at least, it would fail "safe" (separate caches for each function) using this code.

IMO, @memoize_in is more appropriate for nested functions.

Copy link
Contributor

@alexfikl alexfikl Apr 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a small note to the docs that it's really not recommended for nested?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that, it looks good to me! 👍

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytools/__init__.py Outdated Show resolved Hide resolved
pytools/__init__.py Outdated Show resolved Hide resolved
pytools/__init__.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner Author

inducer commented Apr 16, 2021

(I added loopy downstream CI because I happen to know that this change breaks loopy.)

@inducer
Copy link
Owner Author

inducer commented Apr 17, 2021

(I added loopy downstream CI because I happen to know that this change breaks loopy.)

Huh, TIL it doesn't. scratches head All the better. :)

@inducer inducer marked this pull request as ready for review April 17, 2021 18:51
@inducer
Copy link
Owner Author

inducer commented Apr 17, 2021

Turns out the downstream CIs weren't actually using this code:

Requirement already satisfied: pytools from git+file:/home/runner/work/pytools/pytools#egg=pytools in ./.miniforge3/envs/testing/lib/python3.8/site-packages (from -r requirements.txt (line 1)) (2021.2.3)

#82

@inducer
Copy link
Owner Author

inducer commented Apr 18, 2021

There we go. :) Downstream CI is not such a bad thing... when it works.

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

Successfully merging this pull request may close these issues.

2 participants