Skip to content

Consolidate numpy member transforms to reduce function calls#2550

Merged
jacobtylerwalls merged 1 commit intopylint-dev:mainfrom
correctmost:cm/reduce-numpy-transforms
Sep 30, 2024
Merged

Consolidate numpy member transforms to reduce function calls#2550
jacobtylerwalls merged 1 commit intopylint-dev:mainfrom
correctmost:cm/reduce-numpy-transforms

Conversation

@correctmost
Copy link

@correctmost correctmost commented Sep 8, 2024

Type of Changes

Type
✓ 🔨 Refactoring

Description

pylint-dev/pylint#9783 notes that numpy predicates are called millions of times when linting large codebases. I was able to reduce these calls by ~5.9 million when linting yt-dlp by consolidating the transforms.

Stats

I profiled yt-dlp @ ef36d517f9b0 with the import-error rule.

Before

# Total: 62145284 function calls

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
  4560600    0.529    0.000    0.529    0.000 astroid/brain/brain_numpy_utils.py:64(name_looks_like_numpy_member)
  1875144    0.212    0.000    0.213    0.000 astroid/brain/brain_numpy_utils.py:72(attribute_looks_like_numpy_member)
Command Mean [s] Min [s] Max [s] Relative
pylint --recursive=y . 33.301 ± 0.262 32.927 33.765 1.00

After

# Total: 56197047 function calls

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   228030    0.060    0.000    0.060    0.000 astroid/brain/brain_numpy_utils.py:79(member_name_looks_like_numpy_member)
   234393    0.045    0.000    0.046    0.000 astroid/brain/brain_numpy_utils.py:88(attribute_name_looks_like_numpy_member)
Command Mean [s] Min [s] Max [s] Relative
pylint --recursive=y . 32.800 ± 0.345 32.352 33.517 1.00

@codecov
Copy link

codecov bot commented Sep 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (887668b) to head (99186c7).
Report is 109 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2550      +/-   ##
==========================================
- Coverage   92.97%   92.94%   -0.04%     
==========================================
  Files          93       93              
  Lines       11081    11086       +5     
==========================================
+ Hits        10303    10304       +1     
- Misses        778      782       +4
Flag Coverage Δ
linux 92.82% <100.00%> (-0.04%) ⬇️
pypy 92.94% <100.00%> (-0.04%) ⬇️
windows 92.92% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
astroid/brain/brain_numpy_core_function_base.py 100.00% <100.00%> (ø)
astroid/brain/brain_numpy_core_multiarray.py 100.00% <100.00%> (ø)
astroid/brain/brain_numpy_core_numeric.py 100.00% <100.00%> (ø)
astroid/brain/brain_numpy_utils.py 88.57% <100.00%> (-11.43%) ⬇️

@correctmost correctmost mentioned this pull request Sep 8, 2024
Open
Pierre-Sassoulas
Pierre-Sassoulas reviewed Sep 8, 2024
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Is yt-dlp using numpy ? I wonder if some brains should be user activated. (Via an astroid-numpy or pylint-numpy plugin).

Also we might want to deprecate now or even remove now, I doubt those are APIs used by lots of person. Removing in 4.0 seems reasonable (waiting for another input on that, we might just remove now).

@correctmost
Copy link
Author

correctmost commented Sep 8, 2024

yt-dlp doesn't seem to use numpy.

I was hesitant to remove the unused functions from brain_numpy_utils.py because of backward compatibility concerns. Should there be a tracking ticket in the 4.0.0 milestone for API changes?

I'm wondering if some of the API clean-ups needed for typing can happen "in place" like the changes in this PR. The code (and tests) would be messier in the short term, but it might make it easier to make progress without having to maintain multiple branches. The idea needs more investigation, though :).

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Sep 20, 2024

I'll discuss this on discord, but I'd like to advance the main branches of pylint and astroid to 4.0-dev later this month.

@DanielNoord
Copy link
Collaborator

DanielNoord commented Sep 30, 2024

@jacobtylerwalls I think this happened for pylint right? Can we merge this then?

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Sep 30, 2024

Sure thing. I'll follow with a PR do the same in astroid so that @correctmost can resolve the todos by doing the removals.

@jacobtylerwalls jacobtylerwalls added this to the 4.0.0 milestone Sep 30, 2024
@jacobtylerwalls jacobtylerwalls merged commit c7ea1e9 into pylint-dev:main Sep 30, 2024
@correctmost correctmost deleted the cm/reduce-numpy-transforms branch October 2, 2024 07:01
@correctmost correctmost mentioned this pull request Oct 2, 2024
Merged
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 4.0.0, 4.0.0a0 Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas left review comments

@jacobtylerwalls jacobtylerwalls jacobtylerwalls approved these changes

Assignees

No one assigned

Labels

topic-performance

Projects

None yet

Milestone

4.0.0a0

Development

Successfully merging this pull request may close these issues.

4 participants

@correctmost @jacobtylerwalls @DanielNoord @Pierre-Sassoulas