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

Limit inference to a maximum of 100 results at a time #586

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

brycepg
Copy link
Contributor

@brycepg brycepg commented Jul 6, 2018

Prevents spot performance issues.

Add new envrionment variable call ASTROID_MAX_INFERABLE to tune
the max inferable amount of values at a time.

Close #579
Close pylint-dev/pylint#2251

@brycepg brycepg force-pushed the issue-579 branch 2 times, most recently from 11372ca to ea67cd8 Compare July 6, 2018 07:14
Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

This is a great one Bryce! Thank you! Would be great to see what kind of improvement we get on any of our topic-performance issues in pylint. Left two comments, but the more important one is the environment variable that should be in the manager instead.

astroid/util.py Outdated

import importlib
import lazy_object_proxy

MAX_INFERABLE = int(os.environ.get("ASTROID_MAX_INFERABLE", 100))
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually set this in the Manager class, so let's move it there. Also currently it's a bit of a mess, as the options are assigned to the Manager instance directly, but that should be a separate improvement.

astroid/util.py Outdated
exponentially exploding possible results.
"""
count = 0
for result in iterable:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also be something like this:

from itertools import islice

def limit_inference(iterable, size):
   yield from islice(iterable, size)
   has_more = next(iterable, False)
   if has_more is not False:
        yield Uninferable
        return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, looks a lot better

spot performance issues.

Add new envrionment variable call ASTROID_MAX_INFERABLE to tune
the max inferable amount of values at a time.

Close pylint-dev#579
Close pylint-dev/pylint#2251
@brycepg brycepg merged commit c1ba448 into pylint-dev:master Jul 6, 2018
@brycepg brycepg deleted the issue-579 branch July 6, 2018 09:09
@svenpanne
Copy link

Nice! Is there a rough timeline for a pylint release containing this fix? We are actually suffering a bit from pylint's runtime in our project, so any improvement would be highly welcome. We use pipenv to make our environment reproducible, so we rely on released versions. Or is there an easy way to specify a given Git hash in a remote repo in the Pipenv config file?

@PCManticore
Copy link
Contributor

Hey @brycepg did you see my comment regarding os.environ? I don't think this is the way to go, we should have addressed that before this was merged.

@PCManticore
Copy link
Contributor

@svenpanne We'll release 2.0 next week. I actually don't know how to do that in pipenv but should probably allow prerelease versions.

@@ -52,6 +52,8 @@ def __init__(self):
# Export these APIs for convenience
self.register_transform = self._transform.register_transform
self.unregister_transform = self._transform.unregister_transform
self.max_inferable = int(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's default it to 100 by default here, if someone wants to control the value, it should set it against the Manager itself.

@brycepg
Copy link
Contributor Author

brycepg commented Jul 7, 2018

@PCManticore ok i'll change it

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.

3 participants