STYLE enable pylint: method-cache-max-size-none #49751

natmokval · 2022-11-17T18:46:50Z

Issue #48855. This PR enables pylint type "W" warning: method-cache-max-size-none
Based on issue #47371 PERF: Improve Styler to_excel Performance I believe the warning is a false-positive in pandas/io/formats/excel.py#L176.
I disabled this warning on the line 176

All code checks passed.

MarcoGorelli · 2022-11-17T19:11:55Z

Thanks @natmokval - not sure, I think this might be a case where the check might be telling us something useful

@mroeschke do you happen to remember why max_size was set to None, rather than the default 128? #47371 (comment)
The jobs' logs aren't available anymore

mroeschke · 2022-11-17T19:25:20Z

@mroeschke do you happen to remember why max_size was set to None, rather than the default 128? #47371 (comment)

Not too familiar with excel styling, but probably was thinking the opposite, why should the cache size be limited?

Could the caching be refactored to an independent function like in https://pylint.pycqa.org/en/latest/user_guide/messages/warning/method-cache-max-size-none.html?

natmokval · 2022-11-17T19:46:10Z

Thanks @natmokval - not sure, I think this might be a case where the check might be telling us something useful

Thanks for checking @MarcoGorelli. In case I set max_size to 128, what will be the best way to check that my changes do not lead to regression?

natmokval · 2022-11-17T19:48:43Z

Could the caching be refactored to an independent function like in https://pylint.pycqa.org/en/latest/user_guide/messages/warning/method-cache-max-size-none.html?

Thanks, @mroeschke. On the surface I don’t see how the below content can be moved out of the class:

properties = self.compute_css(declarations, self.inherited)
return self.build_xlstyle(properties)

Can you please advise me if you see a direction?

MarcoGorelli

I think we should fix this one, ~~my programming messiah~~ Anthony Sottile has a video on how lru_cache on methods should be avoided: https://youtu.be/sVjtp6tGo0g

I haven't tested this out yet, but I'm "requesting changes" to block this for now

mroeschke · 2022-11-17T21:08:34Z

If I am understanding how this is used from my cursory scan of the code, this is used in to_excel to CSS style each cell of the result but will be cached relative to the number of unique Styler.ctx + Styler.ctx_index + Styler.ctx_columns

So to justify changing maxsize from None to 128 (or similar), is it reasonable to expect a user to provide 100's of CSS declarations? cc @attack68

attack68 · 2022-11-17T21:24:59Z

The number of total possible CSS properties must only be 100-200. So to use them all per cell is never done. V Rare to go more that 10.

Of course potentially lots more independent declarations if going by number of cells, but I'm assuming you meant the former.

MarcoGorelli · 2022-11-17T21:31:06Z

Thanks - let's just go with the default (128) and put @lru_cache() then?

mroeschke · 2022-11-17T22:12:49Z

Thanks - let's just go with the default (128) and put @lru_cache() then?

Sounds good

MarcoGorelli

I looked into this a bit more closely (and trying to repeat what Anthony does in the video I linked), and I think there's a real issue here

For this investigation, I applied the following diff:

diff --git a/pandas/io/formats/excel.py b/pandas/io/formats/excel.py
index a26b85390f..3a8a9ee960 100644
--- a/pandas/io/formats/excel.py
+++ b/pandas/io/formats/excel.py
@@ -170,6 +170,9 @@ class CSSToExcelConverter:
             self.inherited = self.compute_css(inherited)
         else:
             self.inherited = None
+    
+    def __del__(self) -> None:
+        print('heeeelp I\'m being deeeleteeeed')
 
     compute_css = CSSResolver()
 
@@ -193,6 +196,7 @@ class CSSToExcelConverter:
             A style as interpreted by ExcelWriter when found in
             ExcelCell.style.
         """
+        print('I\'m being computed (not looked up in cache)')
         properties = self.compute_css(declarations, self.inherited)
         return self.build_xlstyle(properties)

Once converter.__call__ has been called garbage collection no longer deletes converter!

In [1]: from pandas.io.formats.excel import CSSToExcelConverter

In [2]: import gc

In [3]: converter = CSSToExcelConverter("font-weight: bold")

In [4]: converter('vertical-align: top')
I'm being computed (not looked up in cache)
Out[4]: {'alignment': {'vertical': 'top'}, 'font': {'bold': True}}

In [5]: converter = None

In [6]: gc.collect()  # converter doesn't get deleted!
Out[6]: 373

However, if we then apply this trick from the video:

diff --git a/pandas/io/formats/excel.py b/pandas/io/formats/excel.py
index a26b85390f..692dd23e95 100644
--- a/pandas/io/formats/excel.py
+++ b/pandas/io/formats/excel.py
@@ -170,12 +170,20 @@ class CSSToExcelConverter:
             self.inherited = self.compute_css(inherited)
         else:
             self.inherited = None
+        self._call_cached = lru_cache(maxsize=None)(self._call_uncached)
 
     compute_css = CSSResolver()
 
-    @lru_cache(maxsize=None)
     def __call__(
         self, declarations: str | frozenset[tuple[str, str]]
+    ) -> dict[str, dict[str, str]]:
+        return self._call_cached(declarations)
+
+    def _call_uncached(
+        self, declarations: str | frozenset[tuple[str, str]]
     ) -> dict[str, dict[str, str]]:
         """
         Convert CSS declarations to ExcelWriter style.
@@ -193,6 +201,7 @@ class CSSToExcelConverter:
             A style as interpreted by ExcelWriter when found in
             ExcelCell.style.
         """
         properties = self.compute_css(declarations, self.inherited)
         return self.build_xlstyle(properties)

then it looks like it solves the issue:

In [14]: converter = CSSToExcelConverter("font-weight: bold")

In [15]: converter('vertical-align: top')
I'm being computed (not looked up in cache)
Out[15]: {'alignment': {'vertical': 'top'}, 'font': {'bold': True}}

In [16]: converter('vertical-align: top')  # caching still works
Out[16]: {'alignment': {'vertical': 'top'}, 'font': {'bold': True}}

In [17]: converter = None

In [18]: gc.collect()  # now, it gets deleted!
heeeelp I'm being deeeleteeeed
Out[18]: 698

MarcoGorelli · 2022-11-18T09:03:11Z

Can you please advise me if you see a direction?

I think what needs doing, then is to apply the second diff from #49751 (review) , but:

no need to add the __del__ method (that was just for debugging), nor the other print statement
the docstring of _call_uncached should go to the docstring of __call__
above the assignment self._call_cached = , there should probably be a commenting linking back to this issue, explaining why we avoid lru_cache on the method

pandas/tests/io/formats/test_to_excel.py will also need adjusting slightly, please make sure the tests pass (feel free to reach out on the Slack group if anything trips you up)

natmokval · 2022-11-18T19:24:43Z

Changes from the second diff are applied. pandas/tests/io/formats/test_to_excel.py works with the changes. My local test run passed.

MarcoGorelli

Looks good pending green, well done!

Couple last requests:

this should probably be backported to 1.5.2, could you add a note to the v1.5.2 whatsnew note please? Just say that a memory leak has been fixed, and mention the class
just left a really minor comment

pandas/io/formats/excel.py

Co-authored-by: Marco Edward Gorelli <[email protected]>

natmokval · 2022-11-18T20:19:06Z

Mentioned the memory leak in v1.5.2 whatsnew with issue #48855. Maybe it would be better to skip the number or create a new issue?

doc/source/whatsnew/v1.5.2.rst

Co-authored-by: Matthew Roeschke <[email protected]>

mroeschke

LGTM. cc @MarcoGorelli merge when ready

MarcoGorelli · 2022-11-18T20:53:08Z

Mentioned the memory leak in v1.5.2 whatsnew with issue #48855. Maybe it would be better to skip the number or create a new issue?

The PR number is fine if there's no issue open

Cool, let's wait for tests and we can ship it

mroeschke · 2022-11-18T23:55:40Z

pyproject.toml

@@ -134,7 +134,6 @@ disable = [
  "invalid-envvar-default",
  "invalid-overridden-method",
  "keyword-arg-before-vararg",
-  "method-cache-max-size-none",


@MarcoGorelli have we been using pylint before releasing 1.5? Not sure if this change will cause a merge conflict if backported, and I don't think it's worth backporting pylint configurations to 1.5.x

agree about not backporting the configuration, I'd just backport the fix itself - I'd expect to get a merge conflict, I'll sort this out there

MarcoGorelli

thanks @natmokval !

lumberbot-app · 2022-11-19T07:15:43Z

Oops, something went wrong applying the patch ... Please have a look at my logs.

natmokval · 2022-11-19T08:27:02Z

Thank you @MarcoGorelli and @mroeschke for your help and support. Your explanations helped me to understand the problem.

* STYLE enable pylint: method-cache-max-size-none * STYLE enable pylint: method-cache-max-size-none II * Minor comment update Co-authored-by: Marco Edward Gorelli <[email protected]> * add a note to the v1.5.2 whatsnew * text improvement Co-authored-by: Matthew Roeschke <[email protected]> Co-authored-by: Marco Edward Gorelli <[email protected]> Co-authored-by: Matthew Roeschke <[email protected]>

STYLE enable pylint: method-cache-max-size-none

aa1f4f1

mroeschke added the Code Style Code style, linting, code_checks label Nov 17, 2022

MarcoGorelli suggested changes Nov 17, 2022

View reviewed changes

MarcoGorelli suggested changes Nov 18, 2022

View reviewed changes

natmokval and others added 2 commits November 18, 2022 20:15

STYLE enable pylint: method-cache-max-size-none II

d9df024

Merge branch 'main' into 48855-method-cache-max-size-none

4a6ca39

MarcoGorelli approved these changes Nov 18, 2022 •

edited

Loading

View reviewed changes

MarcoGorelli suggested changes Nov 18, 2022

View reviewed changes

pandas/io/formats/excel.py Outdated Show resolved Hide resolved

MarcoGorelli added this to the 1.5.2 milestone Nov 18, 2022

natmokval and others added 2 commits November 18, 2022 20:53

Minor comment update

db12dc1

Co-authored-by: Marco Edward Gorelli <[email protected]>

add a note to the v1.5.2 whatsnew

87a2f18

mroeschke reviewed Nov 18, 2022

View reviewed changes

doc/source/whatsnew/v1.5.2.rst Outdated Show resolved Hide resolved

text improvement

c32e7c8

Co-authored-by: Matthew Roeschke <[email protected]>

mroeschke approved these changes Nov 18, 2022

View reviewed changes

mroeschke reviewed Nov 18, 2022

View reviewed changes

MarcoGorelli approved these changes Nov 19, 2022

View reviewed changes

MarcoGorelli merged commit 42bb8fe into pandas-dev:main Nov 19, 2022

MarcoGorelli added the Still Needs Manual Backport label Nov 19, 2022

MarcoGorelli mentioned this pull request Nov 19, 2022

Backport PR STYLE enable pylint: method-cache-max-size-none #49784

Merged

5 tasks

MarcoGorelli removed the Still Needs Manual Backport label Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STYLE enable pylint: method-cache-max-size-none #49751

STYLE enable pylint: method-cache-max-size-none #49751

natmokval commented Nov 17, 2022 •

edited by MarcoGorelli

Loading

MarcoGorelli commented Nov 17, 2022

mroeschke commented Nov 17, 2022

natmokval commented Nov 17, 2022

natmokval commented Nov 17, 2022

MarcoGorelli left a comment

mroeschke commented Nov 17, 2022

attack68 commented Nov 17, 2022

MarcoGorelli commented Nov 17, 2022

mroeschke commented Nov 17, 2022

MarcoGorelli left a comment •

edited

Loading

MarcoGorelli commented Nov 18, 2022 •

edited

Loading

natmokval commented Nov 18, 2022

MarcoGorelli left a comment •

edited

Loading

natmokval commented Nov 18, 2022

mroeschke left a comment

MarcoGorelli commented Nov 18, 2022

mroeschke Nov 18, 2022 •

edited

Loading

MarcoGorelli Nov 19, 2022

MarcoGorelli left a comment

lumberbot-app bot commented Nov 19, 2022

natmokval commented Nov 19, 2022

STYLE enable pylint: method-cache-max-size-none #49751

STYLE enable pylint: method-cache-max-size-none #49751

Conversation

natmokval commented Nov 17, 2022 • edited by MarcoGorelli Loading

MarcoGorelli commented Nov 17, 2022

mroeschke commented Nov 17, 2022

natmokval commented Nov 17, 2022

natmokval commented Nov 17, 2022

MarcoGorelli left a comment

Choose a reason for hiding this comment

mroeschke commented Nov 17, 2022

attack68 commented Nov 17, 2022

MarcoGorelli commented Nov 17, 2022

mroeschke commented Nov 17, 2022

MarcoGorelli left a comment • edited Loading

Choose a reason for hiding this comment

MarcoGorelli commented Nov 18, 2022 • edited Loading

natmokval commented Nov 18, 2022

MarcoGorelli left a comment • edited Loading

Choose a reason for hiding this comment

natmokval commented Nov 18, 2022

mroeschke left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Nov 18, 2022

mroeschke Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

MarcoGorelli Nov 19, 2022

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Nov 19, 2022

natmokval commented Nov 19, 2022

natmokval commented Nov 17, 2022 •

edited by MarcoGorelli

Loading

MarcoGorelli left a comment •

edited

Loading

MarcoGorelli commented Nov 18, 2022 •

edited

Loading

MarcoGorelli left a comment •

edited

Loading

mroeschke Nov 18, 2022 •

edited

Loading