Performance regression of Topoly with Python 3.12 #927
Comments
|
I've been doing some profiling and have tracked down the cause of this hang to be a module from Topoly. The attached profile was obtained by running... git switch 4b7d25813
python -m cProfile -o prof/test_run_filters_cProfile_hangs.prof $(which pytest) tests/test_processing.py::test_run_filtersIt can then be visualised with the following (you don't have to run the profiling yourself you can just download the attached file and run the following after installing snakeviz... # Optionally install snakeviz
pip install snakeviz
# Uncompress the file
gunzip test_run_filters_cProfile_hangs.prof.gz
# Run snakeviz on the file
snakeviz path/to/test_run_filters_cProfile_hangs.profA browser tab will open and at the top it shows you that the module where most time is spent is The second box from the left (may differ on your view) with no "icicles" is the
test_run_filters_cProfile_hangs.prof.gz If I repeat this on the previous commit ( To check I've compared the two commits shows this appears to be the first point at which ❱ git diff HEAD HEAD~1 | grep topoly -A3 -B3
❱ git diff HEAD HEAD~1 | grep topoly -A3 -B3
555- 8 8 import numpy.typing as npt
556- 9 9 import pandas as pd
557-10 10 from skimage.morphology import label
558:11 .. from topoly import jones, translate_code
559-12 11
560-13 12 from topostats.logs.logs import LOGGER_NAME
561-14 13 from topostats.tracing.tracingfuncs import coord_dist, genTracingFuncs, order_branch, reorderTrace
--
847-349 if len(nxyz) > 2 and end_to_end_dist_square ...
848-... d <= 2: # pylint: disable=chained-comparison ...
849-350 # single coord traces mean nxyz[0]==[1] ...
850:... so cause issues when duplicating for topoly ...
851-351 nxyz = np.append(nxyz, nxyz[0][np.newax ...
852-... is, :], axis=0) ...
853-352 simple_coords.append(nxyz) ... I'm not sure why this is even being called though as I though git diff --name-only HEAD HEAD~1
topostats/processing.py
topostats/tracing/ordered_tracing.py
topostats/tracing/splining.pyAlthough perhaps my understanding is wrong and everything is getting initalised for some reason. 🤔 @MaxGamill-Sheffield @SylviaWhittle @llwiggins : do you observe this long hang when running the tests? Do you get similar profiling? |
|
Right so I've spent all morning and a bit of the afternoon on this and here's what I found / failed:
All in all I cannot reproduce the hang on the lab windows computer (x86), my computer (ARM - M1), Tom's machine (ARM - M2), Tobi's machine (ARM), Laura's machine (ARM). Did the 1.0.4 release only apply to Unix systems? And can you @ns-rse please try with the downgraded 1.0.2 topoly version as I cannot seem to upgrade? I did also find their website again which says that 1.0.3 should work for ARM, but we see that 1.0.2 works: https://topoly.cent.uw.edu.pl/installation.html |
|
Yep, testing a downgraded version is on my ToDo for Monday. The versions available on PyPI for 1.0.4 are listed here which is why you couldn't install it on your laptop. Should be available for Windows according to that page though. Release History can be browsed to find that the last OSX ARM release was indeed 1.0.2. Will report back Monday. |
|
I've found the root cause of the problem and its related to Python versions not The one test I've been working with now runs in the expected and reasonable amount of time.... With 10 cores I can run the whole test-suite (albeit still with a bunch of failures/errors) in 5m 48sec. This is therefore some sort of performance regression between Topoly and Python 3.12. Before I craft a very long email explaining what I've found to the authors (as they don't have an open repository to report bugs/issues) I'd be grateful if this could be checked on another architecture under Python 3.12 if anyone has time to do so. You can install specific versions of Python using Conda Something like the following should do the trick I think (untested though) but would pull in conda create --name topostats-312 python==3.12.5
conda activate topostats-312
cd path/to/topostats
pip install -e .[dev,tests]
mkdir prof
python --version
pip show pytest
pip show topoly
python -m cProfile -o prof/test_run_filters_cProfile_topoly-1.0.4-python-3.12.prof $(which pytest) tests/test_processing.py::test_run_filters |
|
Mine cannot find an appropriate version of <on |
|
Ah that is expected @SylviaWhittle as we discovered they haven't released wheels for Similar reason to why I couldn't install |
|
@ns-rse When I was testing on on the lab Windows |
|
Yes, checking the release history it seems there are no wheels for Python 3.12 beyond Topoly 1.0.2, it seems to be a bit of a mess really. Seems we can't check. Apologies for not looking more closely I was fitting this in between work on my other projects. I'll craft a report to the authors (which in itself will likely take a couple of hours) and request builds for all architectures across all common Python versions as its going to cause us headaches if this is to remain a key dependency and releases continue in such a disjointed manner. |
|
No this was a really good catch thank you Neil! We'd hate for people to try our software in our upcoming workshop and hit this wall. As a temporary fix could we specify in the docs to use Python <3.12 for now so we can align main with our paper? |
|
I would go a step further and explicitly remove support for Python 3.12 at the package configuration level (i.e. I've added it as a task to #800 and created a separate issue, see #930. I'll leave this issue open until we have resolved support for Python 3.12 (and hopefully 3.13 #928) with Topoly developers/maintainers. |
ns-rse
changed the title
maxgamill-sheffield/topology branch
MaxGamill-Sheffield
changed the title
|
Hi @MaxGamill-Sheffield and @ns-rse my name is Pawel Rubach and I'm responsible for integrating and releasing the topoly package. I was able to reproduce the performance problem you've mentioned here using your code and indeed it only hangs on 3.12. It's difficult to explain since the polvalues.py file that it hangs on doesn't even contain any code, just a list of dictionaries used to translate the polynomials parameters into known topologies. I also ran (on Linux/Ubuntu 24.04) the tests that we use to test topoly before releasing and didn't notice any performance difference between Python 3.11 and 3.12 or even a slightly better performance on 3.12. I managed to build the 1.0.4 version for Windows (it is already on PyPi) and I hope to get the Mac/Apple Silicon version compiled this week. It is difficult since I don't have the hardware at hand and have to remotely run the build process which is pretty complex on my colleagues personal Mac overseas... To further investigate the problem you're facing I'd have to look exactly at how TopoStats uses topoly and try to isolate this functionality. I'll try to look into it in the coming days but if you have any suggestions than that would be welcome. |
|
Thanks for taking the time to investigate this regression test. I found it strange too as I looked at the code and could also see that @MaxGamill-Sheffield will hopefully be able to point you to where and how |
|
By the way @prubach is there a public issue tracker where I might be able to provide more information and track progress? |
|
@ns-rse As to the issue tracker I think the best option for now is to use the topoly_tutorial public repo: https://github.com/ilbsm/topoly_tutorial/issues BTW I've noticed this old issue there and we'll try to address that soon as well, sorry it must have slipped through... |
After merging #897 I checked to see what fails on
maxgamill-sheffield/topologyas I'm working my way through #850 and this branch is due to be merged tomaxgamill-sheffield/800-better-tracing.I found though that whether I ran the tests on a single thread or multiple the test suite never actually starts, it sits there using 100% CPU of the number of cores specified and I have to manually kill it.
git bisectTo investigate this I've employed a
git bisect(using Magit's bisect) runningpytest -xon each bisection.Bisect Log
Bad Commit
This identified the bad commit as...
Of the three files that are changed in this commit there are, at that point, no tests for...
topostats/tracing/ordered_tracing.pytopostats/tracing/splining.py...but there is a test for
topostats/processing.pyand running that in isolation also causes the tests to hang.Trying to run just a single test within this file (
tests/test_processing.py::test_run_filters) took about just over seven minutes so there is something weird going on as that runs in a few seconds on main.Strategy
For each file carefully review the changes...
git diff 4b7d25813e48073099d9b9e9dcd7b02255f13a19 560d0b19d86f38969277002af32cca08920c074 -- <path/to/file.py>Try and identify what has caused the problem, checking by running
pytest tests/test_processing.py. Might be possible to revert some hunks individually to see if they are the cause but never done this.topostats/processing.pyMajority of the changes here are
results_df>grainstats_dfwhich shouldn't have any impact but there are some other difThe text was updated successfully, but these errors were encountered: