Issues with Python wrapped SireMol::AtomProperty<QString> objects

[msuruzhon]

Hi,

I have found an issue quite likely related to property caching of objects that are loaded from serialised data. I am posting a minimal example: Archive.zip

import BioSimSpace as BSS

mol0 = BSS.IO.readMolecules(["methane_vac.parm7", "methane_vac.rst7"])[0]
mol1 = BSS.IO.readMolecules(["methanol_vac.parm7", "methanol_vac.rst7"])[0]
molecule_vac = BSS.Align.merge(mol1, mol0)

# Uncomment the next line, and it will work
# print(molecule_vac._toRegularMolecule())
molecule_vac.save("output")
molecule_vac_new = BSS.Stream.load("output.s3")
print(molecule_vac_new._toRegularMolecule())

Basically, if you call the relevant function before you serialise the system, there is no error. Otherwise, the following error is thrown:

Traceback (most recent call last):
  File "/Users/msuruzhon/Projects/BioSimSpace/test_amber/minimal_example.py", line 11, in <module>
    print(molecule_vac_new._toRegularMolecule())
  File "/Users/msuruzhon/Projects/BioSimSpace/python/BioSimSpace/_SireWrappers/_molecule.py", line 1436, in _toRegularMolecule
    amber_types = mol.property("ambertype1").toVector()
Boost.Python.ArgumentError: Python argument types in
    AtomStringProperty.toVector(AtomStringProperty)
did not match C++ signature:
    toVector(SireMol::AtomProperty<QString> {lvalue}, SireMol::AtomSelection selection)
    toVector(SireMol::AtomProperty<QString> {lvalue})

Many thanks.

[lohedges]

Hi there, the example works for me running locally on Linux, i.e. I see the following output:

<BioSimSpace.Molecule: nAtoms=6, nResidues=1>

What operating system are you using? How did you install BioSimSpace? (Is this a source build, or using conda?)

[msuruzhon]

Thanks for the reply. I am running on macOS using the latest dev conda build.

[lohedges]

It also works with the Linux conda package. Just booting up a macBook to test on macOS.

[lohedges]

Just waiting for the macBook to get enough charge to boot!

Matching your line numbers to my local copy it looks like you are using a modified version of _molecule.py. Is it possible that you've modified the Molecule class in some way so that the state is no longer correctly reconstructed when streaming from file?

[msuruzhon]

That's a good idea! However, I just tried it with the package directly installed from conda, and it still throws me this error.

[lohedges]

I can confirm that I see the same thing on macOS, both with the conda package and when building from source. However, I don't think that this issue is directly related to streaming, or BioSimSpace, since I see similar issues when using Sire directly and trying to access certain molecular properties within the Python, IPython, and sire_python interpreters, e.g.:

from Sire.IO import MoleculeParser
from Sire.Mol import MolIdx

s = MoleculeParser.read(["methane_vac.parm7", "methane_vac.rst7"])
print(s[MolIdx(0)].property("ambertype").toVector())

gives...

ArgumentError: Python argument types in
    AtomStringProperty.toVector(AtomStringProperty)
did not match C++ signature:
    toVector(SireMol::AtomProperty<QString> {lvalue}, SireMol::AtomSelection selection)
    toVector(SireMol::AtomProperty<QString> {lvalue})

If I step back and just print the ambertype property directly, i.e. before using .toVector(), I get the following:

from Sire.IO import MoleculeParser
from Sire.Mol import MolIdx

s = MoleculeParser.read(["methane_vac.parm7", "methane_vac.rst7"])
print(s[MolIdx(0)].property("ambertype"))

ArgumentError: Python argument types in
    AtomStringProperty.__repr__(AtomStringProperty)
did not match C++ signature:
    __repr__(SireMol::AtomProperty<QString>)

In Linux, both of these things work fine. I note that other properties do work fine, e.g. charge, so it looks like something might be off for the wrapping of SireMol::AtomProperty<QString>. (Perhaps some name mangling / de-mangling has gone wrong?) Note that the issue isn't related to printing the object. I can actually assign the property to a variable directly, e.g.:

ambertype = s[MolIdx(0)].property("ambertype")

However, I get errors if I try to call methods of the object, such as .toVector(). (I've not tested all.)

I'll move this issue over to the Sire repository, since it's clearly not related to BioSimSpace itself. I don't think I'll have time to look at this again until early next week.

@chryswoods: Do you have any insight, or suggestions as to how to debug? Given what I'm seeing I'm surprised that the Sire tests on macOS are passing. (I've re-run locally in case there have been dependency updates since the last build.) I guess the problematic properties are only ever being used internally in the C++ layer, rather than being extracted and manipulated in the Python scripts themselves, which is the case in BioSimSpace.

[lohedges]

Actually, I tell a lie, test_lsrc_moves.py does fail. This involves loading an s3 binary file. (This also works on Linux.)

[msuruzhon]

I have had a similar issue on Linux related to strings in C++ and it was ultimately related to some strange clash between SWIG and the C++ 11-style strings. There is some slim chance that something similar might be happening here? Also, from what I remember, these errors definitely don't make much sense and are fairly difficult to track down so I think it will be good to keep that in mind.

[lohedges]

In our case the wrappers are generated using Boost.Python, with a combination of pyplusplus, pygccxml, and castxml used to automate things. We've run into issues before where updates to clang or castxml change the name mangling, so certain objects, or attributes of objects, aren't picked up correctly. When I get a chance I'll simply try rebuilding the wrappers for Sire.Mol to see if there are any changes. Hopefully this is an easy fix once we figure out what's gone wrong.

Cheers.

[lohedges]

Actually, test_lsrc_moves.py does pass. I had forgot to update Sire before recompiling. I've also tried rebuilding the wrappers but the error still persists.

[chryswoods]

This is something that fails occasionally when building the Python wrappers for templated C++ types. Sometimes, the typeid of the templated class is incorrect or inconsistent. This will be because the compiler implemented the templated class in two different namespaces, e.g. ::AtomProperty<QString> and SireMol::AtomProperty<String> as an example. The wrappers end up being compiled against one of these, but then the object is created and has the typeid of the other, hence why you get these errors;

ArgumentError: Python argument types in
    AtomStringProperty.__repr__(AtomStringProperty)
did not match C++ signature:
    __repr__(SireMol::AtomProperty<QString>)

The error is harder to read because aliasing is also used to renamed AtomProperty as AtomStringProperty.

It is extremely difficult to get this all working correctly across all compilers and platforms that Sire supports, especially as this seems to be an edge case that is exposed as compilers or libraries are updated. It is also challenging as the templated classes can be instantiated differently in different libraries (e.g. libSireMol may go one way, but another library may go another) and the code that tries to disentangle all of this is complex.

In the end, my most successful solution was to create concrete classes that inherited from the template classes whenever I wanted to have a property that was properly exposed to Python. Even here, there are some challenges where the methods of the templated class aren't accessible as the typeid of the template class on which the concrete class is derived does not match the typeid of the templated class when it was compiled into the Python wrappers.

This problem has been particularly bad on MacOS, and I never managed to successfully solve it in all cases. My guess is that calling print(molecule_vac._toRegularMolecule()) before the code that breaks causes the C++ objects to be rebuilt or copied, thereby the ::AtomProperty ends up being turned into a SireMol::AtomProperty (or similar).

I am sorry there is not a better fix - exposing templated classes to Python across shared library boundaries is very challenging.

[lohedges]

Thanks for the clear description of the problem. Perhaps there is a way of fixing this internally within BioSimSpace by figuring out how to copy/rebuild the C++ objects on startup. If there is a consistent function call that does this, then we could silently do this behind the scenes.

[lohedges]

Closing since this has been fixed in recent merge.