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.

Sign up for GitHub

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

Removed mutable default value in _inference_tip_cache #1139

Merged
merged 3 commits into from
Aug 27, 2021
+17 −18
Merged

Removed mutable default value in _inference_tip_cache #1139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aad8c8e
Removed mutable default value in _inference_tip_cache
superbobry Aug 19, 2021
90cbac3
Removed the itertools.tee call from _inference_tip_cached
superbobry Aug 22, 2021
9053507
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ What's New in astroid 2.7.2?
Release date: TBA

* ``BaseContainer`` is now public, and will replace ``_BaseContainer`` completely in astroid 3.0.
* The call cache used by inference functions produced by ``inference_tip``
can now be cleared via ``clear_inference_tip_cache``.


What's New in astroid 2.7.1?
Expand Down
33 changes: 15 additions & 18 deletions astroid/inference_tip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,35 @@

"""Transform utilities (filters and decorator)"""

import itertools
import typing

import wrapt

# pylint: disable=dangerous-default-value
from astroid.exceptions import InferenceOverwriteError
from astroid.nodes import NodeNG

InferFn = typing.Callable[..., typing.Any]

_cache: typing.Dict[typing.Tuple[InferFn, NodeNG], typing.Any] = {}


def clear_inference_tip_cache():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I can add a single clear_caches() entry point which also clears LRU caches on various methods in astroid (as discussed in #792). Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using the @lru_cache(maxsize=?) decorator to handle that ? I think it would remove the need for the API. But maybe we'd need a mechanism to set the max size instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd need some extra logic to account for the return value being a generator, right?

Otherwise, lru_cache would do the job. I'd still prefer for us to have an explicit way to clear the cache, because regardless of capacity, references to nodes will keep (potentially expensive) object around in memory for longer.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know lru_cache well I did not know of the potential problem with generator. Just thought that we might as well use a specialized library to have a sane limit without any human intervention (Maybe 8 Go ?). (We can also have the API on top of it by the way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry if I was unclear. I was referring to the bit of logic in the existing implementation, where the cached function does itertools.tee to make the cached generator "re-usable". We could, of course, eagerly materialize the generator into a list/tuple and cached that, instead. Does that sg?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the itertools.tee implementation well enough to guess if it will be better one way or the other. A way to test the speed of an implementation is to use the benchmark test in pylint by doing a local install of astroid and comparing the result. But I think as long as we set a limit to the cache we store it will probably be better than the current situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think itertools.tee has to cache the elements somewhere, and logically if one of the resulting iterators is fully consumed, itertools.tee will have to keep a full copy of the original iterable around internally. With this in mind, it seems we can just drop the itertools.tee and always return a list iterator.

As for lru_cache, I'm afraid we cannot use it here because the current implementation only uses the first argument as key (because the other one is context?), and lru_cache does not allow that.

"""Clear the inference tips cache."""
_cache.clear()


@wrapt.decorator
def _inference_tip_cached(func, instance, args, kwargs, _cache={}): # noqa:B006
def _inference_tip_cached(func, instance, args, kwargs):
"""Cache decorator used for inference tips"""
node = args[0]
try:
return iter(_cache[func, node])
result = _cache[func, node]
except KeyError:
result = func(*args, **kwargs)
# Need to keep an iterator around
original, copy = itertools.tee(result)
_cache[func, node] = list(copy)
return original


# pylint: enable=dangerous-default-value
result = _cache[func, node] = list(func(*args, **kwargs))
return iter(result)


def inference_tip(
infer_function: typing.Callable, raise_on_overwrite: bool = False
) -> typing.Callable:
def inference_tip(infer_function: InferFn, raise_on_overwrite: bool = False) -> InferFn:
"""Given an instance specific inference function, return a function to be
given to AstroidManager().register_transform to set this inference function.

Expand All @@ -54,9 +53,7 @@ def inference_tip(
excess overwrites.
"""

def transform(
node: NodeNG, infer_function: typing.Callable = infer_function
) -> NodeNG:
def transform(node: NodeNG, infer_function: InferFn = infer_function) -> NodeNG:
if (
raise_on_overwrite
and node._explicit_inference is not None
Expand Down