pyvista / pyvista

3D plotting and mesh analysis through a streamlined interface for the Visualization Toolkit (VTK)
https://docs.pyvista.org
MIT License
2.66k stars 489 forks source link

doctest memory increasing #2509

Closed MatthewFlamm closed 2 years ago

MatthewFlamm commented 2 years ago

Describe what maintenance you would like added.

The current usage of doctest has increasing memory. #2496 has increased the number of doctests to the point that the CI runs out of memory. Locally, the memory increases >10 GB on some commit refs. Currently, none of the Plotters are being closed, and thus memory increases each time a plot is made. Confirmation of this is below. However, normal doctest does not allow for convenient use of teardown code. And we'd have to add pyvista.all_close() to every doctest.

pytest-doctestplus can fix this issue while providing more flexible doctesting for our uses. It allows using pytest fixtures, which enables for clean teardown code to be run after each doctest without adding fluff to each doctest.

When using pytest-doctestplus with the fixture below eliminates the memory buildup seen, which confirms that the plotters aren't closing properly today.

@pytest.fixture(autouse=True)
def autoclose_plotters():
    yield
    pyvista.close_all()

Downsides

adeak commented 2 years ago

I'm not sure if this impacts check_doctest_names

check_doctest_names only uses stdlib doctest as a parser, and it only checks for exceptions being raised in doctests (with the assumption that proper doctesting checks validity of results). So it should be of no concern.

akaszynski commented 2 years ago

@MatthewFlamm, I'm finding this works on my end without using pytest-doctestplus

import pyvista

import pytest

@pytest.fixture(autouse=True)
def autoclose_plotters():
    yield
    pyvista.close_all()
adeak commented 2 years ago

@akaszynski indeed seems to work like a charm :tada:

MatthewFlamm commented 2 years ago

I didn't realize that running doctest through pytest would already enable this functionality. This would reduce reliance on niche dependencies.

akaszynski commented 2 years ago

I originally tried this with gc.collect and gave up. You had the idea of close_all, which did the trick. Thanks for that.