Two potential bugs in Cython 0.19

[Gabriel Jacobo]

I've just got two user reports of issues compiling my engine with Cython 0.19 (which are not present in Cython 0.18).

The statement "import time", produces a "cythonization" error of 

import time, sys

      ^

------------------------------------------------------------

backends/sdl/GameLoop.pyx:16:7: Assignment to non-lvalue 'time'

The other error happens when compiling. On certain modules (not all, I haven't figured out the reason why yet) I get ‘PyDateTime_IMPORT’ and PyDateTimeAPI undeclared symbols errors.

Are these bugs or is there some new requirement on Cython 0.19 I'm not complying with? (additional imports required, etc)

Thanks for any help!

--
Gabriel.

[Robert Bradshaw]

We'd really need to see a complete example to help with this.
Certainly a file with nothing but "import time, sys" works just fine.

- Robert

[Gabriel Jacobo]

2013/5/9 Robert Bradshaw <robe...@gmail.com>

You can view the full source on bitbucket, the relevant files are:

First error:

https://bitbucket.org/gabomdq/ignifuga/src/f69301321a75663deed586e02636df05b92d5bb4/src/backends/sdl/GameLoop.pyx?at=default

Second error:

https://bitbucket.org/gabomdq/ignifuga/src/f69301321a75663deed586e02636df05b92d5bb4/src/backends/sdl/Renderer.pyx?at=default

 

I have a bootstrap.py script that gets you easily up to speed if you want to try building the whole engine under Ubuntu and seeing the errors first hand. I understand that you may not want to do this, and I'll try to narrow it down, but any ideas to do so are welcome.

--
Gabriel.

[Stefan Behnel]

Gabriel Jacobo, 09.05.2013 18:39:
> 2013/5/9 Robert Bradshaw

To reproduce this, it's enough to clone the ignifuga repo, rename the "src"
folder to "ignifuga", and then call

cython ignifuga/backends/sdl/GameLoop.pyx

I don't know why the folder is called "src" when the main package is
supposed to be called "ignifuga", but I guess that's not part of the
problem here, given that fixing the name shows the error.

There also seems to be some preprocessing in place, given that I get
additional compile errors like this:

"""
Error compiling Cython file:
------------------------------------------------------------
...
self._screen_w, self._screen_h = self.renderer.screenSize
self.paused = False
self.ticks_second = SDL_GetPerformanceFrequency()

#if DEBUG and (__LINUX__ or __OSX__ or __MINGW__)
self.fw = new FileWatcher()
^
------------------------------------------------------------
ignifuga/backends/sdl/GameLoop.pyx:29:22: Operation only allowed in c++
"""

I wonder why this doesn't just spell as a plain "if DEBUG ...", but I guess
there's a reason. In any case, removing these blocks from the source also
drops their compile errors, just leaving the one in question.

My debugger then tells me that "time" is defined in "cpython/datetime.pxd"
as an extension type, and declared extension types cannot be assigned to.
Looking around a bit more, I see several places that carelessly do "from
cpython cimport *", which obviously leads to massive namespace pollution
that spreads in a hard to controll way. It appears that while "from ...
import *" is just frowned upon in Python code (although heavily so by many
developers), it can actually lead to compile errors and maybe even serious
bugs in Cython code.

The reason why this appeared with 0.19 is that Cython gained its own
cpython/datetime.pxd with several low-level declarations that make this
module more easily accessible. One of those is for the "time" type. Your
star-imports now pick these declarations up and change the way Cython
analyses your code.

So, my advice: clean up your code to make it easier to understand for
yourself and more robust against changes in external code. Star-imports are
discouraged for good reason.

That being said, I still think it's a bad idea that the "cpython" package
uses "from lotsofstuff cimport *" for all underlying submodules. We ran
into this kind of problem at least once already (with "bool" IIRC), and I'm
sure it will keep reoccurring sporadically.

As a fix for this specific problem, I removed the line

from cpython.datetime cimport *

in cpython/__init__.pxd. That line is really wrong, given that "datetime"
is not a core C-API thing but an external module (like the "array" module).

Stefan

[Gabriel Jacobo]

2013/5/10 Stefan Behnel <stef...@behnel.de>

There's a build utility in tools/schafer.py that takes care of building the engine. It takes the stuff inside src, builds it into a "ignifuga" module, patches up the correct paths so Python can find them even though they'll ship as builtins, etc. It's not meant to be manually cythonized though.

 

There also seems to be some preprocessing in place, given that I get
additional compile errors like this:

"""
Error compiling Cython file:
------------------------------------------------------------
...
        self._screen_w, self._screen_h = self.renderer.screenSize
        self.paused = False
        self.ticks_second = SDL_GetPerformanceFrequency()

#if DEBUG and (__LINUX__ or __OSX__ or __MINGW__)
        self.fw = new FileWatcher()
                     ^
------------------------------------------------------------
ignifuga/backends/sdl/GameLoop.pyx:29:22: Operation only allowed in c++
"""

I wonder why this doesn't just spell as a plain "if DEBUG ...", but I guess
there's a reason. In any case, removing these blocks from the source also
drops their compile errors, just leaving the one in question.

The reason is runtime performance on one hand, and lack of support for certain features on certain platforms, which end up in unresolved symbols at link time. I understand preprocessing Python is frowned upon by some developers, but let's agree to disagree here :)

 

My debugger then tells me that "time" is defined in "cpython/datetime.pxd"
as an extension type, and declared extension types cannot be assigned to.
Looking around a bit more, I see several places that carelessly do "from
cpython cimport *", which obviously leads to massive namespace pollution
that spreads in a hard to controll way. It appears that while "from ...
import *" is just frowned upon in Python code (although heavily so by many
developers), it can actually lead to compile errors and maybe even serious
bugs in Cython code.

The reason why this appeared with 0.19 is that Cython gained its own
cpython/datetime.pxd with several low-level declarations that make this
module more easily accessible. One of those is for the "time" type. Your
star-imports now pick these declarations up and change the way Cython
analyses your code.

Thanks, I'll rework the blanket imports and see if that solves the problems. Ideally I would like to support Cython 0.19 "as is" (to avoid having to blacklist that version and making the user figure out how to downgrade, for now), so even if you fix the problem in 0.19.x my code will still be compatible (and cleaner!)

 

So, my advice: clean up your code to make it easier to understand for
yourself and more robust against changes in external code. Star-imports are
discouraged for good reason.

That being said, I still think it's a bad idea that the "cpython" package
uses "from lotsofstuff cimport *" for all underlying submodules. We ran
into this kind of problem at least once already (with "bool" IIRC), and I'm
sure it will keep reoccurring sporadically.

As a fix for this specific problem, I removed the line

   from cpython.datetime cimport *

in cpython/__init__.pxd. That line is really wrong, given that "datetime"
is not a core C-API thing but an external module (like the "array" module).

Thanks a lot!

--
Gabriel.

[Gabriel Jacobo]

2013/5/10 Stefan Behnel <stef...@behnel.de>

That being said, I still think it's a bad idea that the "cpython" package
uses "from lotsofstuff cimport *" for all underlying submodules. We ran
into this kind of problem at least once already (with "bool" IIRC), and I'm
sure it will keep reoccurring sporadically.

As a fix for this specific problem, I removed the line

   from cpython.datetime cimport *

in cpython/__init__.pxd. That line is really wrong, given that "datetime"
is not a core C-API thing but an external module (like the "array" module).

Stefan

I removed all instances of from cpython cimport * from my code and replaced it by a taxative list of symbols (which narrows down to PyObject, Py_XINCREF, Py_XDECREF and Py_CLEAR). The cythonization process completes successfully, but I still get undefined symbol errors when compiling the resulting files:  PyDateTime_IMPORT and PyDateTimeAPI 

Looking at the generated code, these symbols are used inside (as an example) in a function called __pyx_f_7cpython_8datetime_import_datetime which is declared in several files, but it's not actually used anywhere, so I'm having a hard time trying to pinpoint which part of the code is pulling this in and (if at all possible) how to best work around this.

--
Gabriel.

[Zaur Shibzukhov]

среда, 15 мая 2013 г., 18:09:13 UTC+4 пользователь Gabriel Jacobo написал:

Looking at the generated code, these symbols are used inside (as an example) in a function called __pyx_f_7cpython_8datetime_import_datetime which is declared in several files, but it's not actually used anywhere, so I'm having a hard time trying to pinpoint which part of the code is pulling this in and (if at all possible) how to best work around this.

One need to cimport ``import_datetime`` from datetime and "call" them in order to initialize DateTime_CAPI:

   from datetime cimport import_datetime

   import_datetime() 

This works similar to how works numpy.pxd:

  from numpy cimport import_numpy, import_ufunc

  import_numpy()

  import_ufunc()

[Stefan Behnel]

Gabriel Jacobo, 10.05.2013 12:38:

> There's a build utility in tools/schafer.py that takes care of building the
> engine. It takes the stuff inside src, builds it into a "ignifuga" module,
> patches up the correct paths so Python can find them even though they'll
> ship as builtins, etc. It's not meant to be manually cythonized though.

> [...]

> The reason is runtime performance on one hand, and lack of support for
> certain features on certain platforms, which end up in unresolved symbols
> at link time. I understand preprocessing Python is frowned upon by some
> developers, but let's agree to disagree here :)

Well, the obvious downside is that it complicates your build and prevents
straight forward testing. However, since Cython is essentially a
preprocessor all by itself, it's hard to argue that preprocessing Cython
code has a negative impact on e.g. the maintenance of your code.

> Thanks, I'll rework the blanket imports and see if that solves the
> problems. Ideally I would like to support Cython 0.19 "as is" (to avoid
> having to blacklist that version and making the user figure out how to
> downgrade, for now), so even if you fix the problem in 0.19.x my code will
> still be compatible (and cleaner!)

It's common to ship the generated C sources in your releases. That way,
your users do not need Cython at all and you can test the complete code
before the release, instead of relying on users to have the right Cython
version installed, or chasing bugs, compilation problems or performance
regressions that result from them using older versions.

However, this approach is apparently hindered by your pre-processing. My
recommendation is to take another look if you can't move this special
casing to C compilation time, e.g. with a special header file.

Stefan