Bug in Python 3.6

[tic20@…]

Hi all,

Just downloaded the latest build, and upon attempting to use Clipper ran into a crash that appears to be described by the below Python 3.6 bug (discussion includes a patch that was released in mid-Jan):
https://bugs.python.org/issue29327

Relevant part of the traceback:

/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/clipper/crystal.py in __init__(self, session, dataset)
    82     hkl = self.hklinfo = dataset.find_ancestor(db_levels.CRYSTAL_SET)['hkl']
    83     sg = self.spacegroup = hkl.spacegroup
---> 84     cell = self.cell = hkl.cell
    85     res = self.resolution = hkl.resolution
    86     grid = self.grid = clipper.Grid_sampling(sg, cell, res)

/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/clipper/clipper_decorators.py in getter(self)
    87     def property_factory(func):
    88         def getter(self):
---> 89             return getattr(super(self.__class__, self), func)()
    90         prop = property(getter)
    91         return prop

/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/clipper/lib/clipper_python_core/clipper.py in cell(self)
 13561
 13562         """
> 13563         return _clipper.HKL_info_cell(self)
 13564
 13565

SystemError: Objects/tupleobject.c:81: bad argument to internal function

Cheers,

Tristan

[Greg Couch]

Python 3.6.1 is supposed to come out on March 20. That bug is listed as being fixed in the current release candidate, 3.6.1rc1.

[tic20@…]

I *hope* that's the bug, but I'm not entirely sure. The error appears in 
the same place, but I can't see where these functions are calling 
sorted() - in fact, I'm still not entirely clear on where/how 
PyTuple_New gets called at all! SWIG-generated code makes for a heck of 
a maze. I've posted reports on both the SWIG and the Python bug trackers 
to hopefully get some further input.

On 2017-03-08 19:59, ChimeraX wrote:

[Tom Goddard]

I'm not convinced the Python bug report is the same bug you are experiencing -- yours has nothing to do with sort().

Did you completely recompile the clipper Python bindings with Python 3.6, and are you sure there is no Python 3.5 compiled module that is getting used? Because the error looks like the kind of message you might get using a module compiled against Python 3.5 in Python 3.6.

If that is not the issue and you want to make progress on this then you should drop Python 3.6.1rc into your ChimeraX build to see if that fixes it since Greg says the Python bug is supposedly fixed in that release. You should not need to recompile ChimeraX using 3.6.1rc since it is just a patch release beyond the 3.6.0 ChimeraX is being built with.

[Greg Couch]

Specifically, try replacing CHIMERAX/lib/libpython3.6m.so with one from building Python 3.6.1rc1 from source from ​https://www.python.org/downloads/release/python-361rc1/. Use "configure --enabled-shared" to ensure that libpython3.6m.so is built.

[tic20@…]

OK, not a SWIG problem per se. It appears the issue is being triggered 
by this little class decorator, which I've been using extensively to 
wrap the SWIG-generated library into something a little more friendly:

def mappedclass(old_cls):
     '''
      To make things a little more user-friendly than the raw SWIG 
wrapper,
      all the major Clipper classes are sub-classed here with some extra
      documentation, useful Python methods, @property decorators, etc.
      This requires overcoming one small problem: when Clipper itself 
creates
      and returns objects, they're returned as the base class rather than
      the sub-class. So, we have to over-ride the __new__ method for each
      base class to make sure they're instead instantiated as the derived
      class with all the bells and whistles. This function acts as a 
class
      decorator to automate this. For example, for the class Atom derived
      from clipper_core.Atom, place:

      @mappedclass(clipper_core.Atom)

      directly before the Atom class declaration.
     '''

     def decorator(cls):
         def __newnew__(thiscls, *args, **kwargs):
             if thiscls == old_cls:
                 return object.__new__(cls)
             return object.__new__(thiscls)
         old_cls.__new__ = __newnew__

         return cls
     return decorator

Where this worked flawlessly in Python 3.5, now SWIG functions returning 
objects that are wrapped in this way fail.


On 2017-03-09 04:53, ChimeraX wrote:

[tic20@…]

I haven't made much headway on this one, and so far I'm having a little 
trouble convincing the Python people that it's an issue. I've confirmed 
that the below @mappedclass decorator works just fine for pure Python 
functions, but it's somehow breaking SWIG getter functions. Updating to 
3.6.1rc1 didn't help. I just posted the following to both the SWIG and 
Python bug trackers, in hopes that somebody will know what's happening.

If I have two classes Foo and Bar (where Bar has a function get_foo() 
that returns a Foo object) defined in the SWIG-generated library foobar, 
and wrap them as follows:

```
def mappedclass(old_cls):
     '''
     Ensures that objects returned from functions in the clipper_core
     library are instantiated in Python as the derived class with the
     extra functionality.
     '''
     def decorator(cls):
         def __newnew__(thiscls, *args, **kwargs):
             if thiscls == old_cls:
                 return object.__new__(cls)
             return object.__new__(thiscls)
         old_cls.__new__ = __newnew__

         return cls
     return decorator

@mappedclass(foobar.Foo)
class Foo(foobar.Foo):
     pass

@mappedclass(foobar.Bar)
class Bar(foobar.Bar):
     def get_foo(self):
         return super(Bar, self).get_foo()

```

then in Python 3.5:
```
f = Foo()
b = Bar()
b.get_foo()
```
all work. In Python 3.6:

```
f = Foo()
b = Bar()
```
... both work, but
`b.get_foo()`
yields the error as per my OP. Real-world examples:

Constructor (works without trouble in both versions)
```
SWIGINTERN PyObject *_wrap_new_Cell_descr__SWIG_0(PyObject 
*SWIGUNUSEDPARM(self), PyObject *args) {
   PyObject *resultobj = 0;
   clipper::Cell_descr *result = 0 ;

   if(!PyArg_UnpackTuple(args,(char *)"new_Cell_descr",0,0)) SWIG_fail;
   {
     try
     {
       result = (clipper::Cell_descr *)new clipper::Cell_descr();
     } catch (clipper::Message_fatal m)
     {
       SWIG_exception(SWIG_RuntimeError, m.text().c_str() );
       SWIG_fail;
     } catch (std::out_of_range e)
     {
       const char *errString;
       if ( !strcmp(e.what(), "" ) ) {
         errString = "Index out of range!";
       } else {
         errString = e.what();
       }
       SWIG_exception(SWIG_IndexError, errString );
       SWIG_fail;
     } catch (std::length_error e)
     {
       SWIG_exception(SWIG_ValueError, e.what() );
       SWIG_fail;
     } catch (std::invalid_argument e)
     {
       SWIG_exception(SWIG_ValueError, e.what() );
       SWIG_fail;
     } catch (std::exception e)
     {
       SWIG_exception(SWIG_UnknownError, e.what() );
       SWIG_fail;
     } catch (...)
     {
       SWIG_exception(SWIG_UnknownError, "Unknown error" );
       SWIG_fail;
     }

   }
   resultobj = SWIG_NewPointerObj(SWIG_as_voidptr(result), 
SWIGTYPE_p_clipper__Cell_descr, SWIG_POINTER_NEW |  0 );
   return resultobj;
fail:
   return NULL;
}
```

Getter (fails under the above conditions):
```
SWIGINTERN PyObject *_wrap_Cell_cell_descr(PyObject 
*SWIGUNUSEDPARM(self), PyObject *args) {
   PyObject *resultobj = 0;
   clipper::Cell *arg1 = (clipper::Cell *) 0 ;
   void *argp1 = 0 ;
   int res1 = 0 ;
   PyObject * obj0 = 0 ;
   clipper::Cell_descr result;

   if(!PyArg_UnpackTuple(args,(char *)"Cell_cell_descr",1,1,&obj0)) 
SWIG_fail;
   res1 = SWIG_ConvertPtr(obj0, &argp1,SWIGTYPE_p_clipper__Cell, 0 |  0 
);
   if (!SWIG_IsOK(res1)) {
     SWIG_exception_fail(SWIG_ArgError(res1), "in method '" 
"Cell_cell_descr" "', argument " "1"" of type '" "clipper::Cell *""'");
   }
   arg1 = reinterpret_cast< clipper::Cell * >(argp1);
   {
     try
     {
       result = clipper_Cell_cell_descr(arg1);
     } catch (clipper::Message_fatal m)
     {
       SWIG_exception(SWIG_RuntimeError, m.text().c_str() );
       SWIG_fail;
     } catch (std::out_of_range e)
     {
       const char *errString;
       if ( !strcmp(e.what(), "" ) ) {
         errString = "Index out of range!";
       } else {
         errString = e.what();
       }
       SWIG_exception(SWIG_IndexError, errString );
       SWIG_fail;
     } catch (std::length_error e)
     {
       SWIG_exception(SWIG_ValueError, e.what() );
       SWIG_fail;
     } catch (std::invalid_argument e)
     {
       SWIG_exception(SWIG_ValueError, e.what() );
       SWIG_fail;
     } catch (std::exception e)
     {
       SWIG_exception(SWIG_UnknownError, e.what() );
       SWIG_fail;
     } catch (...)
     {
       SWIG_exception(SWIG_UnknownError, "Unknown error" );
       SWIG_fail;
     }

   }
   resultobj = SWIG_NewPointerObj((new clipper::Cell_descr(static_cast< 
const clipper::Cell_descr& >(result))), SWIGTYPE_p_clipper__Cell_descr, 
SWIG_POINTER_OWN |  0 );
   return resultobj;
fail:
   return NULL;
}
```

On 2017-03-09 10:16, Tristan Croll wrote:

[Greg Couch]

If you're ready to try something other than swig, check out pybind11, ​https://github.com/pybind/pybind11/. It's also in pypi. I have no experience with it, but it looks interesting.

[Eric Pettersen]

If you want to change a way a class is constructed (i.e. change new) perhaps you would be better served by using metaclasses instead of decorators. If you are unfamiliar with metaclasses, here is a primer:

​https://jakevdp.github.io/blog/2012/12/01/a-primer-on-python-metaclasses/

The part on "Making Things Interesting: Custom Metaclasses" seems particularly relevant.

--Eric

[Eric Pettersen]

The wiki formatting changed by double-underscore-new-douuble_underscore to an underlined "new"! So now you know what I meant...

[tic20@…]

While my initial reaction was abject horror... that actually looks quite impressive!

 
 
Tristan Croll
Research Fellow
Cambridge Institute for Medical Research
University of Cambridge CB2 0XY
 

 

[tic20@…]

Not sure if metaclasses can do what I want here. The point of this little escapade was to wrap the SWIG output in something a little more Pythonic (getters as properties, allowing arrays/lists/tuples interchangeably in arguments, etc. etc.). But in order for that to work, objects generated within SWIG must be coerced to return as the derived class, *without* ever editing the actual SWIG-generated code. The approach I settled on was working beautifully... until now. As far as I can tell it should *still* be working - it's still perfectly legal Python - but it's breaking somewhere very deep and obscure. 

 
 
Tristan Croll
Research Fellow
Cambridge Institute for Medical Research
University of Cambridge CB2 0XY
 

 

[Tom Goddard]

I just discussed with Greg and Eric how to wrap the SWIG Clipper classes. Your new() approach is one idea, but I am not at all surprised that it has run aground.

The idea we liked the best was that you simply replace SWIG_Clipper_Atom with My_Clipper_Atom when the clipper module is first imported:

import swig_clipper
class My_Clipper_Atom(swig_clipper.SWIG_Clipper_Atom)

...

swig_clipper.SWIG_Clipper_Atom = My_Clipper_Atom

This is not guaranteed to work, because maybe SWIG will somehow use the old SWIG_Clipper_Atom even though you replaced it. But we think that is unlikely (given our ignorance of SWIG internals).

Two other approaches we don't like as much. Simply add your methods and properties and doc strings to the SWIG class

SWIG_Clipper_Atom.my_extra_property = property(...)

Or change the class attribute of instances of SWIG_Clipper_Atom to instead be your derived class:

sca = SWIG_Clipper_Atom()
sca.class = My_Clipper_Atom

All of these may run into problems -- they are all hacks.

[Tom Goddard]

Reassigning bug to Tristan.

[Tom Goddard]

Of course the underlined new and class in my comment 13 are really double underscore new and class.

[tic20@…]

Looks like all will be well, with some more work. After some reading and 
experimentation, I know how to handle the key issues I was trying to 
overcome (in particular, failure to handle certain numpy scalars and 
extension of classes with hand-written Python code) directly in SWIG - 
and the results avoid this error for the test cases I've done so far.  
May as well go ahead and close this ticket.

On 2017-03-09 22:21, ChimeraX wrote:

[goddard@…]

I think you can close the ticket.  Just use an https://… url instead of http://… and use your Chimera account login info, then you will be able to make changes to the ticket including closing it.  If that doesn’t work (maybe there is some other special permissions needed to change tickets), let me know and I’ll close it.

[tic20@…]

If I could make one suggestion/plaintive request, it would be to do away 
with numpy.float32 arrays entirely unless they're absolutely needed for 
performance. The arrays themselves are easy enough to work with, but 
convincing Ctypes and/or SWIG to accept the scalars is an absolute 
nightmare. At present for any C-type function that expects a list of 
scalars, if my input is a numpy.float32 array provided by ChimeraX (e.g. 
Atoms.occupancy) I have to explicitly cast it to float before doing 
anything. It's a real pain in the neck that, as far as I can tell, 
nobody has worked out a really good solution to. Is single precision 
really necessary these days?

T

On 2017-03-09 22:21, ChimeraX wrote:

[goddard@…]

So you mean numpy float64 is easy for SWIG but numpy float32 is not?  It is worth considering where float64 and float32 should be used.  Obviously there is no need for 64-bit precision on occupancy and b-factor and the likes.  Also I have not encountered float32 problems so it seems like a SWIG issue to me.  And SWIG is not very important in the big picture of ChimeraX because very little uses SWIG.  Still I am sympathetic to your problem.  So what are the downsides of 64-bit floats.  A major problem with Chimera 1 is that it uses excessive memory for molecular structures — vastly more than say PyMol or VMD — on the order of 2 to 4 Kbytes per atom, while those more efficient programs can use as little as a couple hundred bytes per atom.  If I change every Atom float32 array to float64 probably it will not make very much difference in memory use.  But if the only reason I do that is because it is inconvenient in crusty old SWIG software, I think it is a bad direction to go in.  I guess my impression is that making you write "occupancy.astype(float64)” instead of “occupancy” in several places and similarly for other float32 arrays in your code is the lesser evil.