-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Limit exception trapping when running under pdb. #9350
Conversation
…is not detected to be present. Fixes pypa#9349.
@@ -186,6 +186,12 @@ def _main(self, args): | |||
"This will become an error in pip 21.0." | |||
) | |||
|
|||
runner = self.run if 'pdb' in sys.modules else self._trap_run |
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.
Ouch, this feels so terribly hacky. Is there no nother way to detect python -m pdb
?
Also, does this also cover alternative pdb frontends e.g. ipdb, pudb?
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.
Very hacky indeed, and maybe even dangerous. I'm unsure if there are environments (Juypter comes to mind) where pdb might have been imported prior to invoking pip. Of course, pip only supports invocation from the command-line, so such a case would likely be unsupported.
I searched around and didn't find anything better. I looked at the pdb code and it doesn't set any state. With Python 3.8, construction of the pdb.Pdb
class does raise a sys.audit
event, but best I can tell, one can only add hooks for these events, not detect them after the fact.
Perhaps one could use the garbage collector to detect the presence of Pdb
instances. That approach would avoid cases where pdb
was imported but not invoked. Would you like me to draft that approach?
Or maybe it would be better to have an explict entry point, where python -m pip.debug
or -m pip.direct
would bypass the error trapping. Such an approach would have the unfortunate restriction of being more difficult to discover. Ideally, the presence of -m pdb
should be enough to enable debugging.
I could also add an escape hatch, an environment variable like PIP_TRAP_ERRORS, which if true would use the trapping behavior, if false would use the direct behavior, but would fall back to the inference above (trap errors except when pdb is detected, however that happens).
My instinct, however, is that this escape hatch or more sophisticated detection behavior is unlikely to be needed, so I'd propose to go with this simple approach for now.
Does this cover alternative pdb frontends?
I checked IPython, and it does import pdb as a matter of course, even when the debugger isn't engaged:
~ $ pip-run -q ipython -- -c "import sys, IPython; print('pdb' in sys.modules)"
True
So that's at least one case where the pdb detection would return a false positive.
That appears not to be the case for pudb:
pip-run --use-pep517 -q pudb -- -c "import sys, pudb; print('pdb' in sys.modules)"
False
Though I did find places where pudb imports bdb
, suggesting that instances of bdb.Bdb
might be the most accurate detection mechanism.
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.
In 501258d, I implement the any(isinstance(ob, bdb.Bdb))
approach.
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.
@jaraco have you tried to check this via gettrace()
? If it is not None
then you're running under some kind of debugger / coverage tool / etc. It might break checks though, also it's only supported by CPython AFAIK, so proceed with caution.
|
||
@staticmethod | ||
def _is_debug(): | ||
return any( |
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.
I measured the cost of this approach. In my environment, it had to iterate over as many as 34k objects and added ~100ms to the startup time. If this approach is the one we would prefer, it probably would be worthwhile to add some instrumentation to bdb.Bdb
to keep track of instances of it.
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.
Uhm, I'm not a big fan of this. I'd much rather that we don't fix this that to add a 100ms cost to every pip execution.
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.
I agree.
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.
I had a thought - what about combining the imperfect "if pdb/bdb in sys.modules" with this code. That would short-circuit early the most common scenario, but allow the more expensive operation to continue.
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.
In b82f628, I add that check and now the startup is fast in the common case.
I think our most reasonable approach is providing either an explicit debug entry point, or a flag (e.g. |
# common case where bdb isn't involved. | ||
return 'bdb' in sys.modules and any( | ||
isinstance(ob, bdb.Bdb) | ||
for ob in gc.get_objects()) |
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.
Isn’t the sys.modules
check always true because you imported it above? 😛
+1 for @uranusjr proposition. Trying to do that automatically will either: a) not work on debuggers other than Edit: typo, I meant "debuggers other than pdb" |
I'm gonna close this PR, because we all have agreed that the approach taken here is not what we want to do. There's a tracking issue for improving pip's debuggability, which is where we can continue the discussion. Thanks for the PR @jaraco, and everyone else for the feedback here. :) |
Fixes #9349.