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

Refactoring of the magics system and implementation of cell magics #1732

Merged
merged 103 commits into from May 27, 2012

Conversation

Projects
None yet
6 participants
@fperez
Member

fperez commented May 14, 2012

This IS now ready to merge!

Implement cell-level magics, with support in all frontends. In all text consoles the behavior is similar to that for other interactive code: one blank line signals the end of the cell. In the notebook, such limitation doesn't exist, of course.

The final version is slightly different from the original IPEP (#1611): we do not allow separate implementations of line and cell magics with the same name. Instead, a single function with signature def f(line, cell=None) (with self if done as a method) will have to be called and distinguish whether the call is line-magic or cell-magic based on whether cell is None or a string.

All tests now pass on my machine, so this is good to go for full review.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 15, 2012

What I think is important is that from the user's perspective there can be cell and line magics with the same name. We don't want to have to have separate %timeit and %timeit_cell magics. I do think that is important? Are you saying you want to back pedal on that? Or are you saying that you want to change the API for implementing those different magics.

In terms of the implementation, I think you should do whatever makes it easiest to accomplish this with minimal complexity and code bloat - even if it doesn't follow the IPEP.

If you feel like you don't have time to support cell magics with the same name right now, then let's come up with a design that will allow us to add that in later without breaking the API we put in place now.

@fperez

This comment has been minimized.

Member

fperez commented May 16, 2012

@ellisonbg, sorry for not being clear enough: there's nothing to worry user-side, it's just that in order to implement a magic that can be used both as %f and %%f, we'll have to write a function with the signature

@line_cell_magic
def f(self, line, cell=None):
  if cell is None:
    do_line_stuff(line)
  else:
    do_line_and_cell_stuff(line, cell)

instead of being able to use separate implementations for the line-only and cell-only parts. So I don't think it's a big deal. I'll add this new decorator.

It's just that I'm finding I need to have a single namespace, so that a magic name X maps to only one function, whether it's line-only, cell-only or line-and-cell. The management of truly disjoint line and cell namespaces was proving to be an ever-growing tangle of complexity.

Does that make sense?

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 18, 2012

Yes, this makes sense. While it is a bit of a pain for people writing
magics, it is not too bad. I think the compromise makes sense if it
simplifies the implementation code, which it sounds like it does.

On Tue, May 15, 2012 at 10:14 PM, Fernando Perez
reply@reply.github.com
wrote:

@ellisonbg, sorry for not being clear enough: there's nothing to worry user-side, it's just that in order to implement a magic that can be used both as %f and %%f, we'll have to write a function with the signature

@line_cell_magic
def f(self, line, cell=None):
 if cell is None:
   do_line_stuff(line)
 else:
   do_line_and_cell_stuff(line, cell)

instead of being able to use separate implementations for the line-only and cell-only parts.  So I don't think it's a big deal.  I'll add this new decorator.

It's just that I'm finding I need to have a single namespace, so that a magic name X maps to only one function, whether it's line-only, cell-only or line-and-cell.  The management of truly disjoint line and cell namespaces was proving to be an ever-growing tangle of complexity.

Does that make sense?


Reply to this email directly or view it on GitHub:
#1732 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk

This comment has been minimized.

Member

minrk commented May 22, 2012

PR #1630 now touches the parallelmagic extension (silent keyword had to be added to run_cell), which will obviously conflict with this. The necessary change was quite tiny, but I included a fair amount of cleanup since that file gets neglected and had gone very stale (docstrings were simply wrong, and not updated since Twisted days, and the run_code override is entirely unnecessary). Should I rollback the cleanup, so resolving conflicts is easier, and doesn't add to the scale of this guy?

@fperez

This comment has been minimized.

Member

fperez commented May 22, 2012

Hey guys (esp. @ellisonbg & @minrk), this has proved to be a fair bit more time-consuming than I was hoping for, but there's now light at the end of the tunnel. At this point, I'm getting the full test suite to pass again (modulo an occasional zmq failure that I only see intermittently and never in isolation, and which is unrelated to magics, so I hope it will get fixed elsewhere).

Note that I still don't actually have the real %%cell syntax implemented, but now this branch again has (to the best of my knowledge) a fully working IPython again. So you can review all this code as a working unit.

I'll now focus on getting the actual cell syntax working. I think it should go comparatively quickly, considering that all the infrastructure is now in place. But I've learned to be cautious with this beast now, so no promises :) I'll work on that tomorrow.

@fperez

This comment has been minimized.

Member

fperez commented May 22, 2012

On Tue, May 22, 2012 at 12:29 AM, Min RK
reply@reply.github.com
wrote:

PR #1630 now touches the parallelmagic extension (silent keyword had to be added to run_cell), which will obviously conflict with this.  The necessary change was quite tiny, but I included a fair amount of cleanup since that file gets neglected and had gone very stale (docstrings were simply wrong, and not updated since Twisted days, and the run_code override is entirely unnecessary).  Should I rollback the cleanup, so resolving conflicts is easier, and doesn't add to the scale of this guy?

Sure, I'd appreciate that so we can knock this monster in one shot.
It should be easy to revisit it once we've merged this, and I'll be
happy to help out if needed.

@fperez

This comment has been minimized.

Member

fperez commented May 22, 2012

Mmh, I wonder why I got @minrk's reply via email but it doesn't show up here on the website. Weird.

@minrk

This comment has been minimized.

Member

minrk commented May 22, 2012

@fperez - I see my note just fine (I posted it on here shortly before your ping, it's not a reply to it).

@minrk

This comment has been minimized.

Member

minrk commented May 22, 2012

Okay, parallelmagic changes in #1630 are now restricted to adding silent kwarg in two places. I will open a separate PR totally rewriting that file after both of these are merged.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 22, 2012

@fperez, OK do you think we should start to review your branch now or is it not quite ready?

I have been reviewing the mergekernel branch and am going to focus on code review. My branch is done until I can rebase on top of your two branches and finish a few things.

@fperez

This comment has been minimized.

Member

fperez commented May 22, 2012

@ellisonbg, go for it. There will be a few more commits with the actual implementation of the cell magics that I'm working on now, but those should be few. If you want to be able to see those in isolation, we can also make them a separate PR later today or tomorrow, in case you go through this in full. At least now this branch works in full and has what I think is the final architecture for magics, modulo missing actual %% cell-level ones.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 22, 2012

OK I will start on the review...

On Tue, May 22, 2012 at 11:54 AM, Fernando Perez
reply@reply.github.com
wrote:

@ellisonbg, go for it.  There will be a few more commits with the actual implementation of the cell magics that I'm working on now, but those should be few.  If you want to be able to see those in isolation, we can also make them a separate PR later today or tomorrow, in case you go through this in full.  At least now this branch works in full and has what I think is the final architecture for magics, modulo missing actual %% cell-level ones.


Reply to this email directly or view it on GitHub:
#1732 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 24, 2012

So I just wrote a simple %%cython_inline magic that passes the cell content to Cython.inline. Here is a notebook that shows this:

https://gist.github.com/2779717

But I am running into a problem. Currently, the return value of a magic is discarded. This means there is no way of getting the result of a computation done in a cell magic back to the user. I think this is something we are going to run into a lot as magics get more complicated and interesting. Any thoughts on getting the return value back to the user? This could also show up if a magic function returns something that has an interesting representation that displayhook/pyout should pick up.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 24, 2012

Here is another version of a %%cython magic that uses pyximport:

https://gist.github.com/2779875

I think this one is more useful.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 24, 2012

I have noticed a common mistake that is not handled very well currently. I keep forgetting to put in the extra % for cell magics. So I will type %cython rather than %%cython. The error message you get in this case is pretty ugly. How should we handle this type of user mistake?

@fperez

This comment has been minimized.

Member

fperez commented May 24, 2012

Could you try pulling again? I've just made a bunch of changes while
you were reviewing...

@certik

View changes

IPython/core/magics/osm.py Outdated
/home/tsuser/parent/child
"""

#bkms = self.shell.persist.get("bookmarks",{})

This comment has been minimized.

@certik

certik May 24, 2012

Why is this line here? It looks like a leftover from some refactoring?

This comment has been minimized.

@fperez

fperez May 24, 2012

Member

Yup, good catch. Fixed locally, will push later today. Thanks!

@certik

View changes

IPython/core/magics/code.py Outdated
filename = make_filename(args)
datafile = 1
warn('Could not find file where `%s` is defined.\n'
'Opening a file named `%s`' % (args,filename))

This comment has been minimized.

@certik

certik May 24, 2012

There should be a space after a comma ((args, filename)), to be consistent.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed.

@certik

View changes

IPython/core/magics/code.py Outdated
filename = make_filename(args)
if filename is None:
warn('The file `%s` where `%s` was defined cannot '
'be read.' % (filename,data))

This comment has been minimized.

@certik

certik May 24, 2012

Space after the comma.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/namespace.py Outdated
vdtype = var.dtype

if vbytes < 100000:
print aformat % (vshape,vsize,vdtype,vbytes)

This comment has been minimized.

@certik

certik May 24, 2012

Space after commas?

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/namespace.py Outdated
if vbytes < 100000:
print aformat % (vshape,vsize,vdtype,vbytes)
else:
print aformat % (vshape,vsize,vdtype,vbytes),

This comment has been minimized.

@certik

certik May 24, 2012

The same here.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/osm.py Outdated

if bkms.has_key(ps):
target = bkms[ps]
print '(bookmark:%s) -> %s' % (ps,target)

This comment has been minimized.

@certik

certik May 24, 2012

Space after comma.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/osm.py Outdated
split = 'l' in opts
out = self.shell.getoutput(cmd, split=split)
if opts.has_key('v'):
print '%s ==\n%s' % (var,pformat(out))

This comment has been minimized.

@certik

certik May 24, 2012

Space after comma.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/osm.py Outdated
fmt = '%-'+str(size)+'s -> %s'
print 'Current bookmarks:'
for bk in bks:
print fmt % (bk,bkms[bk])

This comment has been minimized.

@certik

certik May 24, 2012

Space after comma.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/extensions/storemagic.py Outdated
for var in vars:
justkey = os.path.basename(var)
# print 30 first characters from every var
print fmt % (justkey,repr(get(var,'<unavailable>'))[:50])

This comment has been minimized.

@certik

certik May 24, 2012

Space after comma.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed.

@certik

View changes

IPython/core/magics/auto.py Outdated
else:
arg = 'toggle'

if not arg in (0, 1, 2,'toggle'):

This comment has been minimized.

@certik

certik May 24, 2012

Space after the last comma?

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed

@certik

View changes

IPython/core/magics/basic.py Outdated
mode = parameter_s.split()[0][1:]
if mode == 'rest':
rest_docs = []
except:

This comment has been minimized.

@certik

certik May 24, 2012

What exception is this code trying to catch? IndexError? It should be explicit. I think that a general "try/except" clause like this is a bad programming style.

This comment has been minimized.

@fperez

fperez May 26, 2012

Member

fixed, it looks like an IndexError. Note that this, and the other things you noted above, are remnants of code written ~ 10 years ago, when I was learning python as I developed IPython. So it should be no surprise to see some pretty awful stuff here :) But thanks a ton for the review!

@certik

This comment has been minimized.

certik commented May 24, 2012

I noticed the general "try/except" clause at couple more places -- I think it should be made explicit what exact exception(s) you are trying to catch.

@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

@ellisonbg, I think I'm done with all the items you'd noted. I'm going to look into the py3 stuff now that @takluyver reported, and we should be good to go.

Fix test failures under Python 3.
Refactored the test code a bit for better reuse of common functionality.
@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

OK @takluyver, as best I can tell, I'm also in good shape with Python 3 now, I've applied the compat fix you indicated. Thanks for that review!

fperez added some commits May 27, 2012

Document cell magics in %magic.
%magic is our main interactive explanation of the magic system, so
it's a good place to explain the system.
@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

I've just completed the documentation and added %%timeit as an example of a function that works both as line and cell magic. I'm going to do another pass over the whole thing, but unless anyone spots any more issues, this can go in as far as I'm concerned.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented May 27, 2012

Great, merge away!

Sent from my iPad

On May 26, 2012, at 9:41 PM, Fernando Perezreply@reply.github.com wrote:

I've just completed the documentation and added %%timeit as an example of a function that works both as line and cell magic. I'm going to do another pass over the whole thing, but unless anyone spots any more issues, this can go in.


Reply to this email directly or view it on GitHub:
#1732 (comment)

@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

I've also added %%prun, which was easy and which I figured I'd want right away.

Update the main documentation with new magics API.
Added detailed description to the docs, as well as comprehensive
examples of magic creation with the new APIs.
@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

OK, I've added a bunch of documentation to the actual manual, with detailed examples. Last round of tests, and this goes in...

@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

Test results for commit 0876e92 (can't merge cleanly)
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: matplotlib pymongo qt tornado wx wx.aui zmq)

Not available for testing:

@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

Mmh, I'm not sure why test_pr said above it wouldn't merge cleanly, because it does. Things are looking pretty solid on both 2 and 3, so I'll go ahead and merge now. This is blocking too much stuff, we can (and will) continue to polish on master.

fperez added a commit that referenced this pull request May 27, 2012

Merge pull request #1732 from fperez/cellmagics
Refactoring of the magics system and implementation of cell magics.

This PR completely refactors the magic system, finally moving the magic objects to standalone, independent objects instead of being the mixin class we'd had since the beginning of IPython.  Now, a separate base class is provided in IPython.core.magic.Magics that users can subclass to create their own magics.  Decorators are also provided to create magics from simple functions without the need for object orientation.

All builtin magics now exist in a few subclasses that group together related functionality, and the new IPython.core.magics package has been created to organize this into smaller files.

This cleanup was the last major piece of deep refactoring needed from the original 2001 codebase.

Secondly, this PR introduces a new type of magic function, prefixed with `%%` instead of `%`, which operates at the cell level.  A cell magic receives two arguments: the line it is called on (like a line magic) and the body of the cell below it.

Cell magics are most natural in the notebook, but they also work in the terminal and qt console, with the usual approach of using a blank line to signal cell termination.

This PR closes #1611, or IPEP 1, where the design had been discussed.

@fperez fperez merged commit 61eb2ff into ipython:master May 27, 2012

@takluyver

This comment has been minimized.

Member

takluyver commented May 27, 2012

test_pr examines the 'mergeable' field when it queries the Github API for the pull request. If we see it making a mistake again, I'll check that - it's possible that Github is misreporting it after a rebase, for instance.

Kudos for getting this done. Sorry I didn't have more time to look into it, but I'll do a bit of manual Python 3 testing now.

@takluyver

This comment has been minimized.

Member

takluyver commented May 27, 2012

One small flaw I've noticed: if you start the Qt console, and type %ti<TAB>, it shows the new %%timeit as an option for tab completion, but it reduces the command at the prompt to the longest common prefix, which is now just %. It ought to be smarter about that. (This isn't specific to Python 3)

@takluyver

This comment has been minimized.

Member

takluyver commented May 27, 2012

Everything I've tried with it in Python 3.2 works perfectly.

@Carreau

This comment has been minimized.

Member

Carreau commented May 27, 2012

On Sun, May 27, 2012 at 11:40 AM, Thomas Kluyver <
reply@reply.github.com

wrote:

test_pr examines the 'mergeable' field when it queries the Github API for
the pull request. If we see it making a mistake again, I'll check that -
it's possible that Github is misreporting it after a rebase, for instance.

I've seen that sometime, and for me it seems that gihub api result is
updated only when the PR webpage is opened and the green button 'check'
that the pr is mergeable.

Kudos for getting this done. Sorry I didn't have more time to look into
it, but I'll do a bit of manual Python 3 testing now.


Reply to this email directly or view it on GitHub:
#1732 (comment)

@takluyver

This comment has been minimized.

Member

takluyver commented May 27, 2012

On 27 May 2012 13:00, Bussonnier Matthias
reply@reply.github.com
wrote:

I've seen that sometime, and for me it seems that gihub api result is
updated only when the PR webpage is opened and the green button 'check'
that the pr is mergeable.

Ah, that could well be it. We'll keep an eye out for it, and if that
is the case, we'll drop github a line.

@fperez

This comment has been minimized.

Member

fperez commented May 27, 2012

@takluyver, thanks for catching the completion problem. Opened now as #1767.

@Carreau Carreau referenced this pull request May 29, 2012

Merged

restore loadpy to load #1784

fperez added a commit that referenced this pull request May 29, 2012

Merge pull request #1784 from Carreau/loadpycat2
Restore loadpy to load. 

closes #1783, just the part of #1606 eaten by #1732, where some code was accidentally removed.
@certik

This comment has been minimized.

certik commented May 30, 2012

Great job!

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#1732 from fperez/cellmagics
Refactoring of the magics system and implementation of cell magics.

This PR completely refactors the magic system, finally moving the magic objects to standalone, independent objects instead of being the mixin class we'd had since the beginning of IPython.  Now, a separate base class is provided in IPython.core.magic.Magics that users can subclass to create their own magics.  Decorators are also provided to create magics from simple functions without the need for object orientation.

All builtin magics now exist in a few subclasses that group together related functionality, and the new IPython.core.magics package has been created to organize this into smaller files.

This cleanup was the last major piece of deep refactoring needed from the original 2001 codebase.

Secondly, this PR introduces a new type of magic function, prefixed with `%%` instead of `%`, which operates at the cell level.  A cell magic receives two arguments: the line it is called on (like a line magic) and the body of the cell below it.

Cell magics are most natural in the notebook, but they also work in the terminal and qt console, with the usual approach of using a blank line to signal cell termination.

This PR closes ipython#1611, or IPEP 1, where the design had been discussed.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#1784 from Carreau/loadpycat2
Restore loadpy to load. 

closes ipython#1783, just the part of ipython#1606 eaten by ipython#1732, where some code was accidentally removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment