ENH: add a renderer to save python-profile prof files

[mattip]

Closes #64. Now running python -m pyinstrument -r prof -o <filename> <script> will create a file compatible with python -m profile -o <filename> <script>. It can be loaded into tools like gprof2dot and snakeviz for visualization.

Of course the ncall number is bogus. I used -1. Since we only know total time, the percall time is also not available. Ideas for further tests or correcting the statistics are welcome.

[joerick]

Hey Matti! Thanks for this, it looks good. Would it be possible to add just a test that checks that pstats.Stats doesn't crash when loading the output of a simple script?

[mattip]

I responded to the review and added a test

[mattip]

I think this is ready for another round of review. Perhaps enabling the CI run would give some confidence that the tests are correct?

[joerick]

Thanks and apologies for the delay! I'll try to look in the next few days

[joerick]

This is complicated by the fact that the prof renderer outputs bytes, not strs. I'll have a think how we can resolve this neatly.

[joerick]

I've pushed some fixes to this, if you get a sec could you check this is still working for you @mattip? There were a couple issues with the prof rendering as well as some internal issues (like total_self_time always returning zero), but I think I got them all.

[mattip]

I think this looks good on a toy script I tried. Is there any end-to-end testing you do offline to verify that the renderers all give similar results?

[joerick]

Normally I just play with some of the examples scripts in /examples. I checked these with snakeviz. But it would be good to double check the callers dict somehow, maybe there's some way to visualise the call graph? I seem to remember some tool that had blobs that represent functions and lines representing calls...The other thing we could do is assert more deeply in a unit test, but I'm not sure I'm confident enough how the data should look...! I suppose one could compare a toy script with cProfile to verify.

[mattip]

maybe there's some way to visualise the call graph

Running the code in the new test, and then using gprof2dot

gprof2dot -f pstats /tmp/test_renderer.prof | dot -Tpng -o /tmp/output.png

I get

[mattip]

Sorry, that was cProfile. The ProfRenderer from pyinstrument gives me this. Note the 50% -1 in the call to e and sleep. I wonder what is wrong

[joerick]

nice, thank you! well the "-1" is because the number of calls is -1, due to statistical profiling we don't know that. but the 50% below the branches is odd, and looks wrong. Perhaps it is getting confused with the numcalls being -1? we could try 1 to see if that helps. Otherwise it's probably something to do with the callers dict

[joerick]

Ah, yeah it must be the callers dict - I made the same mistake as we had with the stats dict - the code overwrites rather than adding to the caller entry. I can try a fix now

[joerick]

I thiiiink that might fix it. Are you able to try the viz again?

[mattip]

Yes, that did fix the per-cent. The rendering is a bit off, it is hard to read 100%. But that is certainly not pyinstrument's problem :). I think this is about as good as we can do?

[joerick]

yeah that looks good to me. I'd rather the numcalls said -1 rather than 1 or 0 as it's clear that there is no data.

I think i'd like some more detailed unit tests on the things we've discovered during this PR about the pstats file format. mostly, just basic stuff around frames, timings, and asserting that the fix we made above still holds. If you have time, that would be great, otherwise I'll get around to it at some point.

Something else I'd wondering - is pstats a better name for this than prof?

[mattip]

is pstats a better name for this than prof?

yes, I am horrible at naming things. I will take a look at adding tests. Since the stats output is a dictionary, I think it should be straightforward to assert some of what you suggest.

[mattip]

Added some tests and renamed the renderer.

[joerick]

These test errors can be circumvented using fake time. I can look at that now

[mattip]

Any idea why the readthedocs build failed?

[joerick]

I fixed that in #239, should be resolved by merging main.

[mattip]

Rebased off main, and CI is passing.

[joerick]

Looking over the diff, still a few instances of the renderer referred to as 'prof' and not pstats

[mattip]

Good catch. I changed the file extension for this renderer to *.pstats.

[joerick]

Looks good! Thanks @mattip !

[mattip]

Thanks for shepherding this through to the end.

[mattip]

Is there a plan for a new release?

[joerick]

Sorry for the wait, gonna get #240 in and then cut a new release

[mattip]

Cool, thanks