Skip to content
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

Avoid trying to use IPython unless it seems like it is present #197

Closed
wants to merge 1 commit into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Aug 25, 2016

__IPYTHON__ was apparently removed from only 0.11,
and then the developers got negative feedback and restored it
for 0.12:
ipython/ipython#1059

Various versions of IPython break demand loaders badly.
The current version is 5.1 or thereabouts, I am using 4.x.

It is unlikely that someone would be using 0.11 (July 2011),
so look for this variable first.

I know, this is not the first time we've tried to address IPython. The previous attempt dies as some part of IPython tries to do something that the demand loader isn't expecting. (And the demandloader people thought they'd fixed IPython). This approach works.

__IPYTHON__ was apparently removed from only 0.11,
and then the developers got negative feedback and restored it
for 0.12:
ipython/ipython#1059

Various versions of IPython break demand loaders badly.
The current version is 5.1 or thereabouts, I am using 4.x.

It is unlikely that someone would be using 0.11 (July 2011),
so look for this variable first.
@asmeurer
Copy link
Collaborator

Is this the fix for #177?

# this skips the pain of demandimporters when IPython
# is available but not actually active:
# https://bz.mercurial-scm.org/show_bug.cgi?id=5346
__IPYTHON__
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this can work. The first time IPython is imported is below this line. Wouldn't this only work if PuDB is started from within IPython? We want to support more than just that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this variable exists iff IPython has been imported. Wouldn't 'IPython' in sys.modules be an equivalent check?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess it's rather if an IPython instance exists, which is a stronger condition than IPython just being imported (don't know which is relevant here, as I don't really understand what this fix is doing in any case).

@asmeurer
Copy link
Collaborator

Indeed, I just verified, in this branch when starting PuDB not from IPython (like with ./try-the-debugger.sh), then IPython is not used (it works in master). So -1 to this patch as it stands.

Did you mean to put __IPYTHON__ after the import? I don't understand how this is supposed to fix the issue.

@@ -96,12 +96,19 @@ def have_ipython():
# https://github.com/ipython/ipython/issues/9435

try:
# Assume don't have IronPython unless this variable is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? (IPython doesn't stand for IronPython)

@asmeurer
Copy link
Collaborator

And for the record, I want to reiterate my comment from here that demandimport is really poorly designed on mercurial's part. It should affect only mercurial modules by default, not globally every module. Arbitrary Python libraries shouldn't expected to work with some deep evil builtins hack/magic.

Nonetheless if the fix is simple, isolated, and doesn't change the code semantics, I'm fine with it (the fix here does change the code semantics).

@jsoref
Copy link
Contributor Author

jsoref commented Aug 25, 2016

Ok. so. here's the deal:

Lots of software is installed in an environment.
Lots of it isn't expected to be coming into play just because it's installed. It's expected to come into play at the request of the user (me).

I have python, mercurial, pygments, ipython (for use as a standalone shell), and I have pudb (to debug whatever).

IPython does not play nicely with demandimporters. So, from my perspective as long as nothing uses it, I'm pretty happy. The only thing I expect to use IPython is me -- when I use it from a commandline. Which could include:

``` sh`
$ ipython

import pudb
import mercurial
pudb.set_trace()


Or something of that sort. And, under that condition, it wouldn't upset me if pudb wanted to talk to ipython.

Under all other conditions, I really don't expect it to.

If pudb sees IPython as mandatory, then it shouldn't be trying to handle it not being there.
If pudb sees IPython as optional (which seems likely), then maybe all I need is an "off" switch so that I can tell pudb not to try to use it. That'd certainly save me from constantly fighting it.

My concern is that software seems to think "if it's there, it was definitely put there for me to use" instead of "if it's there, it was perhaps put there for some other reason".

If you're trying to make a helpful debugger, your first goal should be ensuring that it works and doesn't trip over itself or its environment. And I'll gladly help you in this direction.

If you would prefer a patch which enables some form of flag/envvar/pref to disable IPython, please let me know and preferably provide a pointer to something else in pudb that behaves that way, and I'll see about coding it up.

@inducer
Copy link
Owner

inducer commented Aug 25, 2016

What specific problem does this address? (Sorry if I'm being dense)

What's happening now that you think shouldn't?

AFAICT, pudb does exactly what you ask, i.e. unless the shell setting is set to IPython and we're about to start IPython, pudb will not even import IPython.

@asmeurer
Copy link
Collaborator

I just tested setting the shell to "classic" and in the shell, 'IPython' is not in sys.modules.

@jsoref maybe you are missing that you can change the shell in the settings (Ctrl-p)? I believe IPython is the default, but you can set it to something else. If IPython is selected and installed, then it should use IPython if it is installed, even if it isn't imported yet (the current behavior is correct).

@jsoref
Copy link
Contributor Author

jsoref commented Aug 28, 2016

I think the version of pudb I was using was before the Only import IPython upon use commit and I hadn't updated before retesting -- sorry. FWIW, there's no IPython module normally:

(py)sh-4.3$ ipython
iPython 2.7.8 (default, Oct  7 2015, 09:24:26)
Type "copyright", "credits" or "license" for more information.

IPython 4.0.1 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: import sys

In [2]: ', '.join([a for a in sys.modules if a.startswith('ipython')])
Out[2]: 'ipython_genutils.py3compat, ipython_genutils, ipython_genutils.re, ipython_genutils.locale, ipython_genutils.textwrap, ipython_genutils._version, ipython_genutils.errno, ipython_genutils.shutil, ipython_genutils.random, ipython_genutils.text, ipython_genutils.sys, ipython_genutils.importstring, ipython_genutils.functools, ipython_genutils.__builtin__, ipython_genutils.string, ipython_genutils.path, ipython_genutils.encoding, ipython_genutils.warnings, ipython_genutils.os, ipython_genutils.types'

@jsoref jsoref closed this Aug 28, 2016
@jsoref jsoref deleted the demandimport2-ipython branch August 28, 2016 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants