-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Always use the suggestion mode for no-member
/ c-extension-no-member
#9962
base: main
Are you sure you want to change the base?
Always use the suggestion mode for no-member
/ c-extension-no-member
#9962
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the change! Do we need tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Principle makes sense to me.
return "c-extension-no-member", "" | ||
if not self.linter.config.missing_member_hint: | ||
return "no-member", "" | ||
names = _similar_names( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share a profile run for a project like pylint or astroid and let us know the total time spent in _similar_names? That might impact whether we want to lru_cache it, potentially after fiddling with when attname
is filtered out. Let me know if I'm speaking in too much shorthand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hurgh, took ~=50mn to lint pandas (I keyboard interrupted), but I didn't manage to format the output properly for snakeviz so no cool pictures available, but anyway the info about _similar_names
:
Ordered by: cumulative time
ncalls tottime percall cumtime percall filename:lineno(function)
1 0.000 0.000 2997.168 2997.168 __main__.py:1(<module>)
...
571 0.952 0.002 85.079 0.149 typecheck.py:171(_similar_names)
Looks like it's 2.80% of total time, not insignificant.. (50% of the time seems to be spent in calculating string distances).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I also just tried with django, a codebase where I expect no-member
would fire a lot (hence pylint-django
) and got similar results.
Without caching:
1.923s out of 44.409s total (4.3% of time)
With lru_cache():
CacheInfo(hits=168, misses=239, maxsize=512, currsize=239)
1.328s out of 43.122s total (3% of time)
Seems worth it? Maybe a maxsize of 256 or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected the cache to do better than that. Did you use the default ? And on what function ? Maybe it's more impactful to cache the string distance function than the function with the information about the instance. We definitely should cache in any case. (Also, you seem to have a vastly better computer than mine haha, linting Django in 45s ! :star-eyes:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you use the default ?
I increased to maxsize=512, but as you can see the cache only ever grew to 239. Makes sense, there will only ever be so many no-member errors in a mature code base.
And on what function ?
_similar_names()
b370363
to
7a7170f
Compare
This comment has been minimized.
This comment has been minimized.
7a7170f
to
8e103a6
Compare
This comment has been minimized.
This comment has been minimized.
no-member
/ c-extension-no-member
no-member
/ c-extension-no-member
This comment has been minimized.
This comment has been minimized.
return "c-extension-no-member", "" | ||
if not self.linter.config.missing_member_hint: | ||
return "no-member", "" | ||
names = _similar_names( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I also just tried with django, a codebase where I expect no-member
would fire a lot (hence pylint-django
) and got similar results.
Without caching:
1.923s out of 44.409s total (4.3% of time)
With lru_cache():
CacheInfo(hits=168, misses=239, maxsize=512, currsize=239)
1.328s out of 43.122s total (3% of time)
Seems worth it? Maybe a maxsize of 256 or so?
Co-authored-by: Jacob Walls <[email protected]>
Co-authored-by: Jacob Walls <[email protected]>
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit d2abf8d |
Type of Changes
Description
Work in progress to know what others think. There's no reason to add to not suggest imo (?), so we can remove the option too in the spirit of simplification.