Skip to content

CI: re-consider how we test GUI memory leaks #22988

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

Open
tacaswell opened this issue May 6, 2022 · 1 comment
Open

CI: re-consider how we test GUI memory leaks #22988

tacaswell opened this issue May 6, 2022 · 1 comment

Comments

@tacaswell
Copy link
Member

#22002 fixed some (but not all) memory leaks it tkagg as well as added tests to put an upper bound on the amount of memory that lost when making GUI windows in a tight Python loop and only periodically running the event loop.

However after merging we found that these tests were found that the memory consumed had significant variability on OSX on both the OSX and tkagg backends (bug report: #22959 patches: #22987 , #22961 ). While pragmatic, bumping the threshold is giving a reminder of when our image comparisons had some tolerance in them and we ended up using the wrong glyph for a while even though we had an explicit test (it passed with in tolerance!). It is possible I have over-learned this lesson and should chill.

When talking about this on the dev call this week we had at least one wrong idea about the source of the variability and we still do not understand it. Due to the variability it is easily possible that a small leak will not be robustly detectable under the noise and hence the test would not actually test what we want to test. We could get around that my running the tests for much longer (like we do in https://github.com/matplotlib/matplotlib/blob/main/tools/memleak.py) , however that would result in a test that runs for 10s or greater which is very much not ideal for a test suite (either localy or on CI).

To this end we had a couple of ideas:

  • push the memory leakage testing into its own CI job (and only run it on merge / on a clock)?
  • use ASV to run these tests
    • maybe on macstadium, maybe on ARM test box we have, maybe on digital ocean

In the very short term I think we should

  1. merge CI: bump test limit from tkagg on osx #22987
  2. if 1. fails, skip these tests on osx
  3. If we think 2. too much just hiding the bad case under the carpet, pull these tests out and get one of the above ideas done ASAP.
@tacaswell tacaswell added this to the v3.6.0 milestone May 6, 2022
@greglucas
Copy link
Contributor

As a note for the macos failures, taken from: #23059 (comment)

  • Setting the backend to "agg" on a mac currently has a slow leak when installing from source and using the older pinned Freetype version. I grabbed the newest Freetype (2.12.1) and the leak went away on the Agg backend. There was likely an (apparently mac only) leak that was fixed in Freetype sometime over the past several versions, but I couldn't find anything immediately obvious in their changelog, although they have fixed multiple memory leaks.

I'm not sure there is any getting around this memory leak on the mac operating systems (any backend) until the test suite has an updated version of FreeType. If the job does get moved to a new CI run, I suppose we could install a newer version of FreeType for that runner if there aren't any image comparison tests.

@QuLogic QuLogic modified the milestones: v3.6.0, v3.6.1 Sep 14, 2022
@QuLogic QuLogic modified the milestones: v3.6.1, v3.6.2 Oct 6, 2022
@QuLogic QuLogic modified the milestones: v3.6.2, v3.6.3 Oct 27, 2022
@QuLogic QuLogic modified the milestones: v3.6.3, v3.7.0 Jan 11, 2023
@QuLogic QuLogic modified the milestones: v3.7.0, future releases Jan 25, 2023
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

No branches or pull requests

3 participants