Hide IPython internal frame by default.

[Carreau]

This should help with #313

In most case it should have no effects as anyway all the root of the tree is removed as there is no time spent in it. Though with some compiled code and threads, some time may be affected to the root.

[joerick]

I'd love to see a pyisession file for this issue you're hitting in ipython. It seems to me that our ipython support has a bad assumption, the issue can probably be resolved in a more robust/targeted way.

[Carreau]

I agree, unfortunately this is a bug at a client that have sensitive code I'm not allowed to see/use, so even I can't reproduce the issue. Working on this locally required me to patch the source code to have show_all=True.

So I'm not sure what kind of reproducer I could add.

Regardless, beyond this issu, the feedback I got about pyinstrument from the clients are great.

[joerick]

hm.... okay. in that case, can you add this processor only to renderers called from the magic.py file? Could it be more targeted to only trim stems? Also, I think it would be okay to delete the frames entirely, rather than hiding them - this can be done by calling frame.remove_from_parent() and returning the new root frame from the processor.

[Carreau]

Sure, I can try to do that.

[Carreau]

Ok, I added a default processor that strips nothing by default and activate itself when passed the right options (which I pass from the magic).

While I was at it I added a --show-all option to the magic, which leads me to add a trim_stem option in a couple of places; I originally went with session.root_frame(trim_stem=not self.show_all), but it was making some CLI test fail, which expect the stem to still be trimmed, even when --show-all is passed to the CLI.

Do you want me to rebase/squash to clean the commits ?

[Carreau]

Let me know if there is some changes or updates I can include, or if I can help you with something else to free you some time.

[joerick]

I'm not convinced by the adding of trim_stem everywhere - these frames were started before pyinstrument started profiling, and should ideally be hidden. What's your thinking there?

[joerick]

I've made a change to how the processor is included, and how it works, to prevent a crash if the root frame comes from one of those paths listed.

Just a note... I'm not sure how this solution might age, cos the specific code paths and filenames in IPython might change, and we have no reproducer to test with. So I can't promise your issue wont come back... it's more of a bandaid until we can get a reproducer and find out why trim_stem is stopping short in this instance.

[Carreau]

I'm not convinced by the adding of trim_stem everywhere - these frames were started before pyinstrument started profiling, and should ideally be hidden. What's your thinking there?

My thinking is those are the ones I also wan to remove as they appear in the bug my client reported in #313 (the root get assigned some time for I don't know which reason).

I originaly went in some place with session.root_frame(trim_stem=not self.show_all), but this make some CLI unit test fails, and so the only other options was to add a trim_stem kwargs, that then started to spread everywhere.

Just a note... I'm not sure how this solution might age, cos the specific code paths and filenames in IPython might change, and we have no reproducer to test with. So I can't promise your issue wont come back... it's more of a bandaid until we can get a reproducer and find out why trim_stem is stopping short in this instance.

Yes, I'm aware I'm sad I can't really reproduce it; I'm hopping that the core reason of why it happen will become clearer with time, and then I can submit a proper fix. I'm also maintaining IPython so if there are changes in filenames then you can blame on me (and I can blame it on myself). And can send a fix.

[joerick]

My thinking is those are the ones I also wan to remove as they appear in the bug my client reported in #313 (the root get assigned some time for I don't know which reason).

I originaly went in some place with session.root_frame(trim_stem=not self.show_all), but this make some CLI unit test fails, and so the only other options was to add a trim_stem kwargs, that then started to spread everywhere.

Would you be okay with removing that trim_stem option from FrameRenderer? I can't foresee much need for it outside of debugging pyinstrument itself, and I'd rather keep options to a minimum where I can.

Yes, I'm aware I'm sad I can't really reproduce it; I'm hopping that the core reason of why it happen will become clearer with time, and then I can submit a proper fix. I'm also maintaining IPython so if there are changes in filenames then you can blame on me (and I can blame it on myself). And can send a fix.

I had forgotten you maintain ipython :) It's a deal. Thanks for your work on ipython by the way. I use it quite a bit, normally in the console.

[Carreau]

Would you be okay with removing that trim_stem option from FrameRenderer? I can't foresee much need for it outside of debugging pyinstrument itself, and I'd rather keep options to a minimum where I can.

Yeah, I can look into remove it.

I had forgotten you maintain ipython :) It's a deal. Thanks for your work on ipython by the way. I use it quite a bit, normally in the console.

No worries, It's not much maintenance, and if you need anything on IPython itself let me know.
I though of adding a 'is_ipython_frame' in IPython itself, but that might be a bit too much of a tight coupling between pyinstrument and IPython.

[joerick]

Sorry, I wasn't that clear, I meant to remove the trim_stem as a renderer option entirely. Just pushed a commit for that. I'm assuming it was only for debugging purposes on your side?

[Carreau]

Sorry, I wasn't that clear, I meant to remove the trim_stem as a renderer option entirely. Just pushed a commit for that. I'm assuming it was only for debugging purposes on your side?

Ah, ok, no worries. Yes it was mostly to see what it looked like when the step was not trimmed. Thanks