Cython related magic functions #1770
Conversation
| '-f', '--force', action='store_true', default=False, | ||
| help="Force the compilation of the pyx module even if it hasn't changed" | ||
| ) | ||
| @skip_doctest |
There was a problem hiding this comment.
Why the @skip_doctest? There's no doctest in there, so it's not needed.
|
I haven't gone over it in detail yet, but it's looking great! This is going to be really awesome. One quick note: I think we should get into the habit of writing a standalone test file for these. There's a clean enough API now to call the cell magics that writing a proper test should be very straightforward, and it will make it much easier to build trust in the system in the long haul, if each extension has a nice little test file along with it. |
|
I am writing tests and running into the fact that these tests require 1) Cython to be installed (I can test for this) and 2) a working compiler. What do you think is the best way of handling the compiler issues? |
| if not module_name: | ||
| raise ValueError('module name must be given') | ||
| fname = module_name + '.pyx' | ||
| with open(fname, 'w') as f: |
There was a problem hiding this comment.
@takluyver do I need to be more careful about writing the .pyx files for Python 3?
There was a problem hiding this comment.
Not just for Python 3, I think - this is liable to break with non-ascii characters on Python 2 as well. I think the best thing is to use io.open(fname, 'w', encoding='utf-8').
Cython, like Python 3, assumes UTF-8 source code by default, so we don't need an encoding cookie: http://docs.cython.org/src/tutorial/strings.html#source-code-encoding
|
Part of the problem is that I don't have any system w/o a compiler to know how it will fail w/o one. |
|
I figure that the tests should just have a skipif (or rather a file-level test flag so nose ignores the whole file) if Cython isn't importable. If cython is present and no compiler, well, let them fail, since that's a pretty borked setup to have :) |
|
OK I already have the file level test for cython, so we should be set. On Sun, May 27, 2012 at 8:54 PM, Fernando Perez
Brian E. Granger |
| import Cython | ||
| except: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
Instead of a whole file-level indentation, it would be cleaner to use
try:
import cython
except:
__test__ = False
def test()...
That will make nose skip the whole module if cython isn't around, without the ugliness of indenting the whole file.
There was a problem hiding this comment.
I did not know about this, excellent to know!
|
OK I have added docstrings. I have finished everything that I know of so this could be merged or we can wait for additional review. |
|
I'm running test_pr on it now and will report later. In general, it's a good idea to post the test_pr run results with at least 2.7 and 3.2, so we know what both 2.x and 3.x are reasonably covered. |
|
Test results for commit d4163c6 merged into master
Not available for testing: |
|
The log shows the problem: the cythonmagic extension needs to be put into the test machinery with a guard in case the user doesn't have cython installed, so the test suite doesn't blow up on them. Your fixes to the test file are good, the problem is the extension itself. We'll need to have in the main testing code a guard for Cython like we have for every other optional component. |
| @@ -0,0 +1,178 @@ | |||
| # -*- coding: utf-8 -*- | |||
| """ | |||
There was a problem hiding this comment.
The module level docstring should have at least a small summary of what the extension does and the main entry points or useful things it provides.
|
As we add more extensions, I'm realizing we badly need to document them. I know we have no time to go back and fix everything that exists, and the notebook is perfect documentation (though we need to figure out how to include them more clearly in our docs, but that's for another day). So it can be a simple page with just a paragraph and a brief list of the magics, and a sentence that says 'in the directory so and so, a detailed notebook is provided that shows the use of these functions'. Let's create a new section in the docs, Extensions, and drop a short summary of this one to get the ball rolling. That way it will be easier to update later with short descriptions for the others. It shouldn't take more than a few minutes, and once we solve the problem of linking notebooks in the docs, it will serve as a reference point for people to discover what's available, and then go to the individual notebooks with hands-on explanation. |
|
copyright date discussion was in PR #1644 As for notebooks as docs: until we are doing regular sphinx or other export for the notebooks, they have one glaring shortcoming as documentation: they are not searchable. We are adding to documentation, for which none of anyone's standard documentation discovery mechanisms will help. |
|
I've been thinking that the fact that we're writing so much as notebooks is pushing up the priority of finishing nbconvert very very high (and integrating it into the doc build). Because you're completely correct: right now the notebook docs we have are a ghetto we know about, but nobody will find on their own unless they go manually exploring a source download (since binary installs don't even have them). This could be something we could ask on the list to see if anyone has spare cycles to work on: high value and not too hard. |
|
This is looking great. As far as I'm concerned, with a bit more docstrings and the isolation issue fixed for the test suite, it can go in. |
|
OK I will try to work on that tonight. On Tue, May 29, 2012 at 4:38 PM, Fernando Perez
Brian E. Granger |
| """Compile and import everything from a Cython code cell. | ||
|
|
||
| The contents of the cell are written to a `.pyx` file in the | ||
| `~/.cython/magic` using a filename with the hash of the code. |
There was a problem hiding this comment.
Cython does not use a ~/.cython directory at the moment, but what about if it did in the future? Maybe we should stay within our namespace by using something like ~/.ipy_cython_magic?
There was a problem hiding this comment.
Cython does use ~/.cython directory to store caches of compiled code (e.g. ~/.cython/inline), but perhaps it makes more sense to store this stuff in the profile_dir? I don't think we should be creating any new directories outside IPYTHON_DIR.
There was a problem hiding this comment.
Well in that case maybe we should use ~/.cython/ipy_magic ? I see that the bulk of the code is just a modified version of Cython.Build.Inline.cython_inline so this makes me feel more comfortable using ~/.cython/magic.
It's too bad Cython or pyximport doesn't expose an API for this sort module building and caching.
There was a problem hiding this comment.
The reason I went with ~/.cython/magic is that I could imagine that
this extension might someday be maintained and shipped with Cython. I
am fine doing ~/.cython/ipy_magic though as it is more specific.
@minrk are you OK with this or do you really think we should stick
within the profile dir?
On Tue, May 29, 2012 at 7:39 PM, Bradley M. Froehle
reply@reply.github.com
wrote:
- else:
- module = import_module(module_name)
- self._reloads[module_name] = module
- self._import_all(module)
+- @magic_arguments()
- @argument(
- '-f', '--force', action='store_true', default=False,
- help="Force the compilation of the pyx module even if it hasn't changed"
- )
- @cell_magic
- def cython(self, line, cell):
- """Compile and import everything from a Cython code cell.
+- The contents of the cell are written to a
.pyxfile in the~/.cython/magicusing a filename with the hash of the code.Well in that case maybe we should use ~/.cython/ipy_magic ?
Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1770/files#r897576
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
There was a problem hiding this comment.
I don't think it's particularly important. It seems a little bit funky to be using another program's private storage dir, so I might lean slightly towards putting it in the profile dir. We could just ping cython-users, and see what they officially recommend for this kind of behavior.
I don't think the idea that this might some day end up in Cython itself should be relevant to where it caches while it resides in IPython, given that it is such an inconsequential thing to change.
I've also always been a bit more partial to 'ipfoo' than 'ipy_foo' for denoting IPython things, so if it's in .cython, I would prefer 'ipmagic' to 'ipy_magic', but I don't feel strongly (or, I do but I know it's silly, so I will pretend that I don't).
pro profile-dir: IPython only writes data to its own dir
pro .cython: all your Cython caches (that I know of) are in ~/.cython, so trashing all Cython caches is a single rm -rf.
There was a problem hiding this comment.
Thanks for all of this discussion on a relatively trivial point. At this point I'd rank the options as:
~/.cython/ipmagic(or~/.cython/ipy_magic)- IPython profile dir
~/.cython/magic
There was a problem hiding this comment.
Sorry, my verbosity tends to be inversely proportional to the actual significance of the issue at hand.
I pinged cython-users, and they recommended IPython's own dir. This could be per-profile, or even top-level IPYTHONDIR/cythonmagic.
|
My vote would be top-level, so we get maximum cache reuse (if two profiles create the same function, there's no harm in benefitting from reusing the cache). If the cython guys end up later on carrying this, it's easy to move the cache location to be |
|
so |
|
I'm happy without the additional 'magic' there, but don't care too much either way. I'll be OK with either choice, your/Brian's preference. |
* File writes are unicode safe. * Using IPYTHONDIR/cython for code cache.
| args = parse_argstring(self.cython, line) | ||
| code = cell if cell.endswith('\n') else cell+'\n' | ||
| # distutils.Extension cannot handle sources that a unicode | ||
| lib_dir=str(os.path.join(self.shell.ipython_dir,'cython')) |
There was a problem hiding this comment.
@takluyver @fperez and @minrk how should I handle the fact that distutils.Extension cannot handle unicode paths? Right now to get it to work I am just calling str on this path.
There was a problem hiding this comment.
How about calling X.encode(encoding), with encoding='utf-16' on windows and utf-8 elsewhere? If we can believe this post, Windows uses utf-16 uniformly for filesystem encoding, so that should work.
There was a problem hiding this comment.
Here's a Stack overflow post confirming the above, so that should be a good approach.
There was a problem hiding this comment.
Shouldn't it be sys.getfilesystemencoding()?
Also, we should be careful that it is str, not bytes, since presumably py3 distutils isn't this broken.
This is a stupid bug in distutils - I commented out the type checking in distutils, and the inlining with unicode paths works perfectly. Unfortunately, I don't see a clean way of monkeypatching to just fix the checks.
There was a problem hiding this comment.
Can you check to see what py3 distutils expects here?
On Wed, May 30, 2012 at 8:37 PM, Min RK
reply@reply.github.com
wrote:
- The contents of the cell are written to a
.pyxfile in the- directory
IPYTHONDIR/cythonusing a filename with the hash of the code.- This file is then cythonized and compiled. The resulting module
- is imported and all of its symbols are injected into the user's
- namespace. The usage is similar to that of
%%cython_pyximportbut- you don't have to pass a module name::
+- %%cython
- def f(x):
- return 2.0*x
- """
- args = parse_argstring(self.cython, line)
- code = cell if cell.endswith('\n') else cell+'\n'
- # distutils.Extension cannot handle sources that a unicode
- lib_dir=str(os.path.join(self.shell.ipython_dir,'cython'))
Shouldn't it be
sys.getfilesystemencoding()?Also, we should be careful that it is str, not bytes, since presumably py3 distutils isn't this broken.
This is a stupid bug in distutils - I commented out the type checking in distutils, and the inlining with unicode paths works perfectly. Unfortunately, I don't see a clean way of monkeypatching to just fix the checks.
Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1770/files#r905304
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
There was a problem hiding this comment.
But wait , it has to be str, not bytes
Sent from my iPad
On May 31, 2012, at 8:08 AM, Fernando Perezreply@reply.github.com wrote:
The contents of the cell are written to a `.pyx` file in thedirectory `IPYTHONDIR/cython` using a filename with the hash of the code.This file is then cythonized and compiled. The resulting moduleis imported and all of its symbols are injected into the user'snamespace. The usage is similar to that of `%%cython_pyximport` butyou don't have to pass a module name::
%%cythondef f(x):return 2.0*x"""args = parse_argstring(self.cython, line)code = cell if cell.endswith('\n') else cell+'\n'# distutils.Extension cannot handle sources that a unicodelib_dir=str(os.path.join(self.shell.ipython_dir,'cython'))@takluyver, thanks! We actually looked at the py3compat utilities, but since we don't know them too well, we didn't quite pick the right approach. Why don't you just go ahead and apply this fix straight up into master? Things like that are OK in my mind to do post-review without needing the overhead of a full-blown PR, they're just minor polish that would have happened during review with a bit more time.
Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1770/files#r908331
There was a problem hiding this comment.
cast_bytes_py2 casts to bytes (which is the same as str) on Python 2, and doesn't do anything on Python 3.
There was a problem hiding this comment.
OK sounds good.
On Thu, May 31, 2012 at 12:40 PM, Thomas Kluyver
reply@reply.github.com
wrote:
- The contents of the cell are written to a
.pyxfile in the- directory
IPYTHONDIR/cythonusing a filename with the hash of the code.- This file is then cythonized and compiled. The resulting module
- is imported and all of its symbols are injected into the user's
- namespace. The usage is similar to that of
%%cython_pyximportbut- you don't have to pass a module name::
+- %%cython
- def f(x):
- return 2.0*x
- """
- args = parse_argstring(self.cython, line)
- code = cell if cell.endswith('\n') else cell+'\n'
- # distutils.Extension cannot handle sources that a unicode
- lib_dir=str(os.path.join(self.shell.ipython_dir,'cython'))
cast_bytes_py2casts to bytes (which is the same as str) on Python 2, and doesn't do anything on Python 3.
Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1770/files#r910556
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
* Added extension to the Sphinx docs. * Fixed unicode issues.
|
@fperez: OK I think I have done everything. If all passes on py3, it is ready to merge. One other point: I can't build the docs on OS X. I get the following error:
|
|
On Wed, May 30, 2012 at 10:04 PM, Brian E. Granger
Running the tests now...
Mmh, that's odd. Let me rerun here, though I'm pretty sure they did |
|
Yep I did a make clean first. On Wed, May 30, 2012 at 10:09 PM, Fernando Perez
Brian E. Granger |
|
Test results for commit c80e366 merged into master
Not available for testing: |
|
Mmh, ignore that py3 failure, I have no idea why test_pr is picking up a 2.x matplotlib! @takluyver, I would have thought your recent PYTHONPATH fixes to test_pr would take care of that. Does this failure make sense to you? |
|
So the test failure looks like it is related to the 2.7 version of matplotlib being used in python 3. Yes, as you were saying... |
|
OK, I'm merging this: we have no major problems left, and at this point more work here hits diminishing returns, IMO. We can always polish further in master. Brian, great job, thanks!! |
|
Yipee, cython! On Wed, May 30, 2012 at 10:16 PM, Fernando Perez
Brian E. Granger |
Cython related magic functions: offers the new cell magics %%cython_inline, %%cython_pyximport and %%cython to make it very easy to put cython-accelerated code in a cell and have it loaded interactively.
Cython related magic functions: offers the new cell magics %%cython_inline, %%cython_pyximport and %%cython to make it very easy to put cython-accelerated code in a cell and have it loaded interactively.
This is the first version of Cython related magic functions. An example notebook is provided. I am still finishing the docstrings, but the code should be done.