-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optional debug info to ophydobj with inspect #961
base: main
Are you sure you want to change the base?
Conversation
@klauer, @prjemian, @danielballan, @tacaswell, what are your thoughts about it? I remember we discussed something along these lines with Ken long time ago, but I forgot where, and what was the result. |
A few thoughts:
|
If we can solve this from the outside, like with It’s possible that |
adb50f8
to
e7d08b6
Compare
I created a demo repository to test this feature: https://github.com/mrakitin/profile_inspect_ophyd_pr961. The
This is addressed via e7d08b6, good suggestion, Ken!
It does slow things down. You could check the timing report at the bottom of the https://github.com/mrakitin/profile_inspect_ophyd_pr961#with-ophyd_debug_with_inspect1 section. Posting it here for quicker access: # Without OPHYD_DEBUG_WITH_INSPECT=1
In [1]: durations
Out[1]:
{'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/00-base.py': 0.565570912000112,
'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/01-devices.py': 0.0007143089997043717}
# With OPHYD_DEBUG_WITH_INSPECT=1
In [1]: durations
Out[1]:
{'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/00-base.py': 42.23476164300064,
'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/01-devices.py': 0.009821958999964409} So, we have a ~40s overhead in this simple case, therefore I prefer to have this feature optional, and disabled by default.
Please check https://github.com/mrakitin/profile_inspect_ophyd_pr961#to-address-questions-about-edit-and-obj-ipython-magics. Unfortunately, the magic commands do not work for the objects defined in the profile startup files. |
40+ seconds?! Wow... I wonder if it's related to adding this information onto every ophyd areadetector class and such. |
Good suggestion! It indeed speeds it up quite a bit: In [1]: durations
Out[1]:
{'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/00-base.py': 0.665280208000695,
'/home/vagrant/.ipython/profile_inspect_ophyd_pr961/startup/01-devices.py': 0.006712106000122731} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me overall. It could be useful 👍
ophyd/ophydobj.py
Outdated
@@ -10,6 +11,11 @@ | |||
from .log import control_layer_logger | |||
|
|||
|
|||
OPHYD_DEBUG_WITH_INSPECT = os.getenv('OPHYD_DEBUG_WITH_INSPECT') | |||
if OPHYD_DEBUG_WITH_INSPECT: | |||
import inspect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small standard-library import - I'd say include it in the normal import block above.
You might consider something like this to avoid confusion about how to enable it:
os.environ.get("...", "") in ("y", "yes", "1")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add a better check soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks useful to me 👍
ophyd/ophydobj.py
Outdated
@@ -10,6 +11,11 @@ | |||
from .log import control_layer_logger | |||
|
|||
|
|||
OPHYD_DEBUG_WITH_INSPECT = os.getenv('OPHYD_DEBUG_WITH_INSPECT') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you imagine this being the only thing we'll want to observe more closely in a "slow" debug build?
If not, we'll probably end up renaming this later- the naming is very specific to this one case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, @ZLLentz! We may have other use-cases for inspect
, so I think naming it as OPHYD_DISCOVER_CLASSES_SOURCE
(or something similar) will reflect the purpose of the environment variable. I probably also need to add it to the documentation 🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, going the other direction and making it OPHYD_DEBUG
could make it easier to switch on/off general debugging tools. I think @ZLLentz may have been pointing that way.
I am looking for a quick way to extract the information from what file an ophyd object was instantiated, and where its source code is. It can be done with
inspect.stack()
. It causes performance issues within__init__subclass__
, so I decided to make it optional, enabled via an environment variableOPHYD_DEBUG_WITH_INSPECT
. That can be useful for debugging.Usage: