use standard IPython startup instead of embed in manage.py shell #512

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@minrk

IPython.embed has different namespace/initialization behavior, probably none of which is actually intended or desirable.

This changes manage.py shell to start a standard IPython shell instead, and works for any IPython ≥ 0.11.

See Ticket 17078 for reference.

@minrk minrk use standard IPython startup instead of embed
embed has different namespace/initialization behavior,
none of which is actually desirable.
ce23122
@aaugustin
Django member

From past experience, each way to embed IPython has advantages and drawbacks. We've already bounced a few times. The difficult part isn't writing the code, it's explaining why it is suitable.

@minrk

Sure, sorry it wasn't clearer (both here and in the severely lacking IPython docs on the matter).

This code is the 'standard' way to start IPython. It is the same code executed when you type ipython at the command-line.

There are two basic ways to start IPython:

  1. normal (user namespace is __main__)
  2. embedded (namespace is specified by the caller)

The main difference between these two is in how they initialize the user namespace. For the normal mode, the user namespace is initialized from config files, startup files, etc. The IPython namespace is __main__, etc. Users who customize their environments generally expect these things to happen when they have a 'regular' IPython session. The whole point of embedded IPython is to skip all of that, and allow the caller to setup the namespace instead (think Pdb.set_trace(), but with a few extra options). A side point is that the embedded mode is also more buggy than the normal one, largely due to how it shoves itself into the calling namespace, and its relative lack of use.

There are advantages to the embedded mode, but django is using none of them. The main effect of django using IPython.embed is that the IPython namespace is actually embedded in django's Command.ipython instance method, and user namespace configuration and startup files are ignored. I expect that neither of these are actually intended.

Further, this is actually the ≥ 0.11 equivalent of the IPShell() code used for old IPython. I think it was merely a misunderstanding / failure of documentation that led to the use of IPython.embed here.

One possible permutation:

This code explicitly ignores command-line args (as in IPShell below). If you want to relay args to IPython for startup, then you could pass the appropriate list where this code has argv=[].

@aaugustin
Django member

I hadn't noticed you were a developer of IPython -- sorry! Thanks for the detailed explanation. It makes sense to me, and it'll be useful as a future reference if someone complains about the change in behavior.

@ramiro
Django member

@minrk
Do you envision some practical way to implement a test for this fix were one to add it together with the fix?

Been playing with the idea and I think I have the ipython invocation if present part sorted out. But I don know what would be a good way to assert it is e.g. loading user preferences.

If we don't find a good way maybe we should we simply commit the fix w/o tests.

@minrk

It's always a bit messy testing interactive things, what with starting and communicating with subprocesses, but one way to test would be to start manage.py shell, then do:

In [1]: print get_ipython().__class__.__name__

Before this fix, the name will be 'InteractiveShellEmbed', after, it will be 'TerminalInteractiveShell'.
The bug here was that an embedded shell was used, so an appropriate test might be assert 'Embed' not in output (it would not be appropriate to do an affirmative assert, since the correct name is not guaranteed to be permanent).

@ramiro
Django member

Fixed in 3570ff7. Thanks!

@ramiro ramiro closed this Dec 31, 2012
@minrk minrk deleted the minrk:ipython branch Jan 1, 2013
@ThiefMaster ThiefMaster added a commit to indico/indico that referenced this pull request Feb 29, 2016
@ThiefMaster ThiefMaster shell: use proper ipython startup
See django/django#512 for a detailed explanation why this startup method
is better.  Basically it ensures that the shell behaves like your
regular ipython shell instead of e.g. not running your ipython startup
files.
86f8420
@ThiefMaster ThiefMaster added a commit to indico/indico that referenced this pull request Apr 12, 2016
@ThiefMaster ThiefMaster shell: use proper ipython startup
See django/django#512 for a detailed explanation why this startup method
is better.  Basically it ensures that the shell behaves like your
regular ipython shell instead of e.g. not running your ipython startup
files.
b92643c
@nanuxbe nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016
@timgraham timgraham Fixed #512 -- Invalidated cached blog post on save. aff8620
@nanuxbe nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016
@timgraham timgraham Refs #512 -- Fixed cached blog post invalidation. 3b66595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment