joerick/pyinstrument

Followup for IPython integration

telamonian opened this issue · 2 comments

I got some feedback from the guys I implemented #217 for:

For:

$ cat profiler.py
import time

def D():
    time.sleep(0.7)

def C():
    __tracebackhide__ = True
    time.sleep(0.1)
    D()

def B():
    __tracebackhide__ = True
    time.sleep(0.1)
    C()

def A():
    time.sleep(0.1)
    B()

A()

Profiler output:

1.004 <module>  <string>:1
   1.004 run_path  runpy.py:245
      1.001 _run_module_code  runpy.py:89
         1.001 _run_code  runpy.py:63
            1.001 <module>  profiler.py:1
               1.001 A  profiler.py:16
                  0.700 D  profiler.py:3
                     0.700 sleep  None
                        [2 frames hidden]  <built-in>
                  0.301 sleep  None
                     [2 frames hidden]  <built-in>

  1. Inside A, we would like to see that a few frames (B&C) were hidden because of __tracebackhide__. We also suggest including a >way to expand hidden nodes in HTML format.
  2. I see, sleep None. Should it be <built-in> instead? [Note: I don't see None in Jupyter notebook].

I'm not entirely sure what of the above is bugs, intended behavior, or new feature requests. The request for expandable hidden nodes in html format seems like a reasonable new feature. I can start looking into making that happen.

As for "Inside A, we would like to see that a few frames (B&C) were hidden because of __tracebackhide__", I think what they mean is that when a frame is hidden due to the __tracebackhide__ rule, the output should explicitly say that.

@joerick Can you please clarify exactly what the above example profiling output is currently saying about the hidden frames? I assume the first [2 frames hidden] <built-in> line is refering to the B and C scopes. Is that right? In that case, the <built-in> label is a bug, and should say <ipython built-in> (or something) instead. What does the second repeat of the [2 frames hidden] <built-in> refer to?

Can you please clarify exactly what the above example profiling output is currently saying about the hidden frames?

The [2 frames hidden] should mean that there are library frames here (not application frames) that are hidden by default. That functionality is represented in the data model as a 'Group'. But I also agree that there is a bug here... I get the following-

pyinstrument /tmp/profilet.py     main

  _     ._   __/__   _ _  _  _ _/_   Recorded: 19:04:16  Samples:  5
 /_//_/// /_\ / //_// / //_'/ //     Duration: 1.021     CPU time: 0.002
/   _/                      v4.4.0

Program: /tmp/profilet.py

1.019 <module>  profilet.py:1
└─ 1.019 A  profilet.py:16
   ├─ 0.704 D  profilet.py:3
   │  └─ 0.704 sleep  None
   │        [2 frames hidden]  ..
   └─ 0.315 sleep  None
         [2 frames hidden]  ..

But if i disable the hiding feature, I get-

  _     ._   __/__   _ _  _  _ _/_   Recorded: 19:04:43  Samples:  4
 /_//_/// /_\ / //_// / //_'/ //     Duration: 1.017     CPU time: 0.002
/   _/                      v4.4.0

Program: /tmp/profilet.py

1.016 <module>  profilet.py:1
└─ 1.016 A  profilet.py:16
   ├─ 0.704 D  profilet.py:3
   │  └─ 0.704 sleep  None
   └─ 0.312 sleep  None

To view this report with different options, run:
    pyinstrument --load-prev 2022-11-22T19-04-43 [options]

That seems wrong, those hidden frames aren't shown, they have now disappeared. Also, you're right that the sleep frame's source should be 'built-in' rather than None.

Perhaps the traceback_hide is causing some issues with the hide feature. If you have time to look into this, I'd be grateful. First thing I'd try is changing the order of the processors, perhaps traceback-hiding should come before grouping.

  • Inside A, we would like to see that a few frames (B&C) were hidden because of __traceback_hide__. We also suggest including a >way to expand hidden nodes in HTML format.

This is perhaps possible, but awkward due to how hiding is implemented at the moment. With the functionality of __traceback_hide__, I was following the stacktrace example, where they are just removed, as if they never existed at all.

I think all the issues raised here are now fixed!