Merge lp:~ipython-dev/ipython/set_trace into lp:ipython/0.11

Hi,

here is my set_trace() patch. Please review it and let me know what you think. See also these emails to the ipython list:

http://mail.scipy.org/pipermail/ipython-dev/2009-June/005191.html

see also the whole thread:

http://mail.scipy.org/pipermail/ipython-dev/2009-June/thread.html#5191

This should probably not be in __init__.py. It is intended for those who embed ipython, i.e. public api. Therefore it should be in ipapi.py.

> This should probably not be in __init__.py. It is intended for those who embed
> ipython, i.e. public api. Therefore it should be in ipapi.py.

Many thanks for the review, I'll fix it. Should I fix it by adding a new commit, or by changing the last commit (how can this be done with bzr?)?

> > This should probably not be in __init__.py. It is intended for those who
> embed
> > ipython, i.e. public api. Therefore it should be in ipapi.py.
>
> Many thanks for the review, I'll fix it. Should I fix it by adding a new
> commit, or by changing the last commit (how can this be done with bzr?)?

Just continue committing to this branch and push. You can't amend the old commit.

> > > This should probably not be in __init__.py. It is intended for those who
> > embed
> > > ipython, i.e. public api. Therefore it should be in ipapi.py.
> >
> > Many thanks for the review, I'll fix it. Should I fix it by adding a new
> > commit, or by changing the last commit (how can this be done with bzr?)?
>
> Just continue committing to this branch and push. You can't amend the old
> commit.

Ok, I have moved it to IPython/ipapi.py. So now it has to be imported with:

IPython.ipapi.set_trace()

do you think I can even import it in IPython/__init__.py? Or should I not do that?

One more question --- I would like to change my email in the last two commits, is there a way to do it in bzr? It uses ondrej@crow, but I'd like to use my real email address.

> > > > This should probably not be in __init__.py. It is intended for those who
> > > embed
> > > > ipython, i.e. public api. Therefore it should be in ipapi.py.
> > >
> > > Many thanks for the review, I'll fix it. Should I fix it by adding a new
> > > commit, or by changing the last commit (how can this be done with bzr?)?
> >
> > Just continue committing to this branch and push. You can't amend the old
> > commit.
>
> Ok, I have moved it to IPython/ipapi.py. So now it has to be imported with:
>
> IPython.ipapi.set_trace()
>
> do you think I can even import it in IPython/__init__.py? Or should I not do
> that?
>
> One more question --- I would like to change my email in the last two commits,
> is there a way to do it in bzr? It uses ondrej@crow, but I'd like to use my
> real email address.

I have fix the emails, see the ipython list for how I did it.

Ondrej,

I have seen two problems with the docstring, it is not valid rst and will not render ok with sphinx.

The first code example looks like it should be an example at a common python prompt, but then it needs a proper >>> prompt.

The second code example will render on a single line. To get an in-line code example use a double colon at the end of the previous line.

/Jörgen

Hi Ondrej,

sorry for not having worked on this earlier...

What I don't understand is what the point of this feature is, when we already have the embedding classes as explained here:

http://ipython.scipy.org/doc/stable/html/interactive/reference.html#embedding

Your code basically reduces the code needed to embed ipython from 3 to 2 lines, but the existing machinery actually does it with much more flexibility. So I'd like to understand the justification for this feature better before considering it for merging. Ville's and Jorgen's comments still apply, but I'm actually wondering if we need this *at all*. It seems to me that we already have tools for this, though I may well be missing something and would be happy to reconsider.

> Hi Ondrej,
>
> sorry for not having worked on this earlier...
>
> What I don't understand is what the point of this feature is, when we already
> have the embedding classes as explained here:
>
> http://ipython.scipy.org/doc/stable/html/interactive/reference.html#embedding
>
> Your code basically reduces the code needed to embed ipython from 3 to 2
> lines, but the existing machinery actually does it with much more flexibility.
> So I'd like to understand the justification for this feature better before
> considering it for merging. Ville's and Jorgen's comments still apply, but
> I'm actually wondering if we need this *at all*. It seems to me that we

Thanks for the suggestion, this oneliner does exactly what I want:

from IPython.Shell import IPShellEmbed; IPShellEmbed()()

The only reason why I devised my line and then contributed a patch is that noone has either replied or suggested anything better so far, up until you.

> already have tools for this, though I may well be missing something and would
> be happy to reconsider.

There is only one thing that I am still missing --- It's really annoying to always type:

from IPython.Shell import IPShellEmbed; IPShellEmbed()()

and it still annoying to type:

import IPython; IPython.ipapi.set_trace()

I would much prefer to just type:

import IPython; IPython.embed()

I use this feature *very* often. Would you consider such a patch?

Ondrej

> Thanks for the suggestion, this oneliner does exactly what I want:
>
> from IPython.Shell import IPShellEmbed; IPShellEmbed()()
>

Thanks for catching this Fernando. Until recently, I haven't been very
familiar with our embedding code.

> The only reason why I devised my line and then contributed a patch is that
> noone has either replied or suggested anything better so far, up until you.
>
> > already have tools for this, though I may well be missing something and
> would
> > be happy to reconsider.
>
> There is only one thing that I am still missing --- It's really annoying to
> always type:
>
> from IPython.Shell import IPShellEmbed; IPShellEmbed()()
>
> and it still annoying to type:
>
> import IPython; IPython.ipapi.set_trace()
>
> I would much prefer to just type:
>
> import IPython; IPython.embed()
>

What would that do? I

> I use this feature *very* often. Would you consider such a patch?
>

If it simplifies embedding, then yes, I think we would consider it. But, it
should go into Shell.py, which I have just refactored for the 0.11 release
(won't go into 0.10). Are you OK waiting for the 0.11 for this - it will be
much cleaner to implement then.

Cheers,

Brian

> Ondrej
> --
> https://code.edge.launchpad.net/~ipython-dev/ipython/set_trace/+merge/8855<https://code.edge.launchpad.net/%7Eipython-dev/ipython/set_trace/+merge/8855>
> You are subscribed to branch lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

> > Thanks for the suggestion, this oneliner does exactly what I want:
> >
> > from IPython.Shell import IPShellEmbed; IPShellEmbed()()
> >
>
> Thanks for catching this Fernando. Until recently, I haven't been very
> familiar with our embedding code.
>
>
> > The only reason why I devised my line and then contributed a patch is that
> > noone has either replied or suggested anything better so far, up until you.
> >
> > > already have tools for this, though I may well be missing something and
> > would
> > > be happy to reconsider.
> >
> > There is only one thing that I am still missing --- It's really annoying to
> > always type:
> >
> > from IPython.Shell import IPShellEmbed; IPShellEmbed()()
> >
> > and it still annoying to type:
> >
> > import IPython; IPython.ipapi.set_trace()
> >
> > I would much prefer to just type:
> >
> > import IPython; IPython.embed()
> >
>
> What would that do? I

From the user perspective it would be exactly equivalent to:

from IPython.Shell import IPShellEmbed; IPShellEmbed()()

> > I use this feature *very* often. Would you consider such a patch?
> >
>
> If it simplifies embedding, then yes, I think we would consider it. But, it
> should go into Shell.py, which I have just refactored for the 0.11 release
> (won't go into 0.10). Are you OK waiting for the 0.11 for this - it will be
> much cleaner to implement then.

Sure, I can move it whenever is the best and sure I can wait for 0.11. Are you ok with me adding a stub into the __init__.py, so that this can be invoked just by:

import IPython; IPython.embed()

?

Ondrej

On Mon, Jul 27, 2009 at 9:08 PM, Ondrej Certik<email address hidden> wrote:
>> If it simplifies embedding, then yes, I think we would consider it.  But, it
>> should go into Shell.py, which I have just refactored for the 0.11 release
>> (won't go into 0.10).  Are you OK waiting for the 0.11 for this - it will be
>> much cleaner to implement then.
>
> Sure, I can move it whenever is the best and sure I can wait for 0.11. Are you ok with me adding a stub into the __init__.py, so that this can be invoked just by:
>
> import IPython; IPython.embed()

Yes, I'm also OK with having a simplified embed() be part of the
top-level public API. But it should at least take a few arguments.
Basically you are 'folding' into a single embed() call the creation of
 an IPShellEmbed instance and its activation:

embed() <==> IPShellEmbed()()

But if we look at the signatures for IPShellEmbed's constructor and
activation, we see:

    def __init__(self,argv=None,banner='',exit_msg=None,rc_override=None,
                 user_ns=None):
    def __call__(self,header='',local_ns=None,global_ns=None,dummy=None):

so I'd suggest making the embed() signature something that reasonably
merges the two:

def embed(argv=None,banner='',exit_msg=None,rc_override=None,
     header='',local_ns=None,global_ns=None)

where the relevant arguments are fed through to either the __init__ or
the __call__ as appropriate.

This makes it consistent with the API it's wrapping for convenience
(only where it makes sense, so things like dummy which are meant for
the stateful part of the object aren't needed in this form).

With such caveats, I have no objections to exposing this at the
top-level. It is a very useful feature, so having the one-liner
being even easier to remember is a net win as far as I see it.

Thanks!

f

> On Mon, Jul 27, 2009 at 9:08 PM, Ondrej Certik<email address hidden> wrote:
> >> If it simplifies embedding, then yes, I think we would consider it.  But,
> it
> >> should go into Shell.py, which I have just refactored for the 0.11 release
> >> (won't go into 0.10).  Are you OK waiting for the 0.11 for this - it will
> be
> >> much cleaner to implement then.
> >
> > Sure, I can move it whenever is the best and sure I can wait for 0.11. Are
> you ok with me adding a stub into the __init__.py, so that this can be invoked
> just by:
> >
> > import IPython; IPython.embed()
>
> Yes, I'm also OK with having a simplified embed() be part of the
> top-level public API. But it should at least take a few arguments.
> Basically you are 'folding' into a single embed() call the creation of
> an IPShellEmbed instance and its activation:
>
> embed() <==> IPShellEmbed()()
>
> But if we look at the signatures for IPShellEmbed's constructor and
> activation, we see:
>
> def __init__(self,argv=None,banner='',exit_msg=None,rc_override=None,
> user_ns=None):
> def __call__(self,header='',local_ns=None,global_ns=None,dummy=None):
>
> so I'd suggest making the embed() signature something that reasonably
> merges the two:
>
> def embed(argv=None,banner='',exit_msg=None,rc_override=None,
> header='',local_ns=None,global_ns=None)
>
> where the relevant arguments are fed through to either the __init__ or
> the __call__ as appropriate.
>
> This makes it consistent with the API it's wrapping for convenience
> (only where it makes sense, so things like dummy which are meant for
> the stateful part of the object aren't needed in this form).
>
> With such caveats, I have no objections to exposing this at the
> top-level. It is a very useful feature, so having the one-liner
> being even easier to remember is a net win as far as I see it.

Excellent, I am glad we are on the same boat now. I will play with this and try to produce a new patch. I will create the method embed(<args as you suggested>), add it to Shell.py and import it from __init__.py.

Ondrej

On Tue, Jul 28, 2009 at 3:12 AM, Fernando Perez<email address hidden> wrote:

>> import IPython; IPython.embed()
>
> Yes, I'm also OK with having a simplified embed() be part of the
> top-level public API. But it should at least take a few arguments.

IPython.ipapi is the top level public api, not IPython. Starting to
shove stuff to IPython package namespace is messy, and the line should
be drawn somewhere.

If the aim is to reduce typing, provide a root level module that gives
you access to stuff in IPython package (import ipy; ipy.embed() )

--
Ville M. Vainio
http://tinyurl.com/vainio

On Tue, Jul 28, 2009 at 1:39 AM, Ville M. Vainio<email address hidden> wrote:
> IPython.ipapi is the top level public api, not IPython. Starting to
> shove stuff to IPython package namespace is messy, and the line should
> be drawn somewhere.

This is meant for 0.11, where we are refactoring the entire layout of
the package. I have no problem whatsoever with some commonly used
names being exposed at the top level, and embedding is certainly a
common enough task.

I actually probably look up the 'standard one-liner' from the docs
more than anything else, so I already see value in having it as an
instantly rememberable/discoverable top-level name.

> If the aim is to reduce typing, provide a root level module that gives
> you access to stuff in IPython package (import ipy; ipy.embed() )

Nope. This, we will most certainly not do, at all.

Packages should not go about polluting the top level *Python* pacakge
namespace, which is a shared resource, for little things like this.

> On Tue, Jul 28, 2009 at 3:12 AM, Fernando Perez<email address hidden> wrote:
>
> >> import IPython; IPython.embed()
> >
> > Yes, I'm also OK with having a simplified embed() be part of the
> > top-level public API. But it should at least take a few arguments.
>
> IPython.ipapi is the top level public api, not IPython. Starting to
> shove stuff to IPython package namespace is messy, and the line should
> be drawn somewhere.
>
> If the aim is to reduce typing, provide a root level module that gives

Yes, the aim is to reduce typing (e.g. I was using IPython.ipapi.set_trace() and it's quite pain. IPython.embed() is way better).

> you access to stuff in IPython package (import ipy; ipy.embed() )

Do you mean to provide the root level module in IPython, or in all my other programs?

The way I see it is to put one line into IPython/__init__.py

from Shell import embed

(or something similar). I don't think it's messy --- it's something that users will call frequently.

Ondrej

On Tue, Jul 28, 2009 at 4:50 AM, Ondrej Certik<email address hidden> wrote:

>> you access to stuff in IPython package (import ipy; ipy.embed() )
>
> Do you mean to provide the root level module in IPython, or in all my other programs?

I meant creating a module that you install in your site packages,
which only does:

from IPython.ipapi import embed

This seems more hygienic than importing the name to IPython package.
Ultimately, I don't think the root level package should import
anything else - but we can easily agree to disagree on this, I don't
really care about it one way or another - just think it's a tad messy.

--
Ville M. Vainio
http://tinyurl.com/vainio

A few comments:

* for 0.11, the actual embed implementation should go into ipapi or shell,
which ever makes most sense. shell is now very cleaned up and it really
only has things that qualify as being "public". Thus, it might make sense
for it to go there.
* There should definitely be an import of embed into IPython.__init__.
Things like this shield users (at some level) from changes to the
implementation of things.
* The first time embed gets called it should create the IPShellEmbedded
instance and *save* it as some global like, shell._embedded_shell (this
makes me think ipapi makes more sense!) and then call it. Future calls to
embed should just call the same instance over again. I want to make sure
that embed can be called multiple times.
* Trunk will be ready for this as of Friday this week.

Cheers,

Brian

On Tue, Jul 28, 2009 at 2:51 AM, Ville M. Vainio <email address hidden> wrote:

> On Tue, Jul 28, 2009 at 4:50 AM, Ondrej Certik<email address hidden> wrote:
>
> >> you access to stuff in IPython package (import ipy; ipy.embed() )
> >
> > Do you mean to provide the root level module in IPython, or in all my
> other programs?
>
> I meant creating a module that you install in your site packages,
> which only does:
>
> from IPython.ipapi import embed
>
> This seems more hygienic than importing the name to IPython package.
> Ultimately, I don't think the root level package should import
> anything else - but we can easily agree to disagree on this, I don't
> really care about it one way or another - just think it's a tad messy.
>
> --
> Ville M. Vainio
> http://tinyurl.com/vainio
> https://code.launchpad.net/~ipython-dev/ipython/set_trace/+merge/8855<https://code.launchpad.net/%7Eipython-dev/ipython/set_trace/+merge/8855>
> You are subscribed to branch lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

> A few comments:
>
> * for 0.11, the actual embed implementation should go into ipapi or shell,
> which ever makes most sense. shell is now very cleaned up and it really
> only has things that qualify as being "public". Thus, it might make sense
> for it to go there.
> * There should definitely be an import of embed into IPython.__init__.
> Things like this shield users (at some level) from changes to the
> implementation of things.
> * The first time embed gets called it should create the IPShellEmbedded
> instance and *save* it as some global like, shell._embedded_shell (this
> makes me think ipapi makes more sense!) and then call it. Future calls to
> embed should just call the same instance over again. I want to make sure
> that embed can be called multiple times.
> * Trunk will be ready for this as of Friday this week.

Cool, please ping me when your branch is ready, so that I can take it and update the embed() code according to this thread.

Ondrej

Ondrej,

Trunk is completely ready for you to finish your work in this branch. At this point we should be able to merge this quickly.

These capabilities are now in trunk. See the function IPython.embed.

> Ondrej,
>
> Trunk is completely ready for you to finish your work in this branch. At this
> point we should be able to merge this quickly.