search

OpenBioSim / sire

Sire Molecular Simulations Framework
https://sire.openbiosim.org
GNU General Public License v3.0
41 stars 11 forks source link

Fix issue #215 #216

Closed lohedges closed 3 months ago

lohedges commented 3 months ago

This PR closes #215 by setting the IFBOX pointer to 3 for triclinic spaces. The value of 2 is reserved for a truncated octahedron. I'm not sure if we should add logic to detect this, or whether the use of 3 should suffice for everything. I'll test and report back if there are any issues.

Note that I've killed this superses #214, so I've killed the CI for that PR.

This also closes #217 by switching to the librdkit-dev package as a replacement for the now deprecated rdkit-dev.

Suggested reviewers:

@chryswoods

lohedges commented 3 months ago

It looks like the same version was installed for both the build and tests (dev not for the tests):

    rdkit:                                2024.03.5-py312h7b4b7d0_1  conda-forge
    rdkit-dev:                            2024.03.5-ha4b6fd6_0       conda-forge

However, when building the wrappers it states:

-- Found RDKit library files at $PREFIX/lib/libRDKitGraphMol.so;$PREFIX/lib/libRDKitRDGeneral.so;$PREFIX/lib/libRDKitSmilesParse.so;$PREFIX/lib/libRDKitRDGeometryLib.so;$PREFIX/lib/libRDKitDistGeomHelpers.so;$PREFIX/lib/libRDKitForceField.so;$PREFIX/lib/libRDKitForceFieldHelpers.so;$PREFIX/lib/libRDKitSubstructMatch.so
-- Could NOT find RDKit (missing: RDKIT_INCLUDE_DIR)

So, it looks like Sire doesn't work with this version of RDKit. Will install locally to try to work out what the problem is. Locally I have:

rdkit                     2024.03.4       py312h93d94ad_0    conda-forge
rdkit-dev                 2024.03.4            hcaeca22_0    conda-forge
lohedges commented 3 months ago

Okay, this is because the patch release version of RDKit appears to have removed most of the required headers, i.e.:

2024.03.4:

ls -l .conda/envs/openbiosim/include/rdkit                                                                                      ✘ 130
total 96
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 Catalogs
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 ChemicalFeatures
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 CIPLabeler
drwxr-sr-x  3 lester lester  4096 Jul  9 14:23 DataManip
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 DataStructs
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 DistGeom
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 Features
drwxr-sr-x  4 lester lester  4096 Jul  9 14:23 ForceField
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 Geometry
drwxr-sr-x 42 lester lester  4096 Jul  9 14:23 GraphMol
drwxr-sr-x  5 lester lester  4096 Jul  9 14:23 Numerics
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 Query
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 RDBoost
drwxr-sr-x  3 lester lester  4096 Jul  9 14:23 RDGeneral
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 RDStreams
drwxr-sr-x  2 lester lester  4096 Jul  9 14:23 SimDivPickers
-rw-r--r--  2 lester lester 29440 Jul 26  2019 RingDecomposerLib.h

2024.03.5:

ls -l .conda/envs/debug/include/rdkit/
total 4.0K
drwxr-sr-x 3 lester lester 4.0K Jul 25 09:27 RDGeneral

The life of a conda package maintainer. I'll raise an issue on their feedstock.

lohedges commented 3 months ago

As per #217, rdkit-dev has been deprecated in favour of librdkit-dev. I've updated our requirements accordingly, which now means that we only have librdkit-dev as a host requirement and a runtime dependency on an appropriate rdkit version. (Nothing related is in build.) I now see the following in the CI logs so I'm hoping things are fixed:

-- Found RDKit library files at $PREFIX/lib/libRDKitGraphMol.dylib;$PREFIX/lib/libRDKitRDGeneral.dylib;$PREFIX/lib/libRDKitSmilesParse.dylib;$PREFIX/lib/libRDKitRDGeometryLib.dylib;$PREFIX/lib/libRDKitDistGeomHelpers.dylib;$PREFIX/lib/libRDKitForceField.dylib;$PREFIX/lib/libRDKitForceFieldHelpers.dylib;$PREFIX/lib/libRDKitSubstructMatch.dylib
-- Found RDKit: $PREFIX/include/rdkit
lohedges commented 3 months ago

Oh, I give up. As soon as I fix the RDKit issue it now appears that gemmi isn't being detected:

CMake Warning at Convert/SireGemmi/CMakeLists.txt:77 (message):
  gemmi not found, so we cannot compile the converter.

I'll check back through the logs to see how long this has been happening, but it appears that the devel release might not have gemmi support.

lohedges commented 3 months ago

Nope, no gemmi errors for the last merge to devel, so this is new too.

lohedges commented 3 months ago

The version has not changed since yesterday (0.6.6) when it was detected during a previous CI run for this PR, so I'm not sure why it's not working. It certainly doesn't depend on RDKit.

lohedges commented 3 months ago

It seems like this fix won't work for SOMD1 since the old amber.cpp code makes some invalid assumptions about the value when it's set to 3:

    if (pointers[IFBOX] == 1)
    {
        /** Rectangular box, dimensions read from the crd file */
        Vector dimensions(crd_box[0], crd_box[1], crd_box[2]);

        // qDebug() << "We have a periodic box of dimensions"
        //          << crdBox[0] << crdBox[1] << crdBox[2];

        // PeriodicBox.
        if (crd_box[3] == 90.0 && crd_box[4] == 90.0 && crd_box[5] == 90.0)
        {
            spce = PeriodicBox(dimensions);
        }
        // TriclinicBox.
        else
        {
            spce = TriclinicBox(dimensions.x(), dimensions.y(), dimensions.z(), crd_box[3] * degrees,
                                crd_box[4] * degrees, crd_box[5] * degrees);
        }
        // spce = PeriodicBox( Vector ( crdBox[0], crdBox[1], crdBox[2] ) ).asA<Space>() ;
        // qDebug() << " periodic box " << spce.toString() ;
    }
    else if (pointers[IFBOX] == 2)
    {
        /** Truncated Octahedral box*/
        throw SireError::incompatible_error(QObject::tr("Sire does not yet support a truncated octahedral box"),
                                            CODELOC);
    }
    else
    {
        /** Default is a non periodic system */
        spce = Cartesian();
    }

The second clause will need to be set to 2 or 3 to catch truncated octahedron and general triclinic spaces.

lohedges commented 3 months ago

Have replaced with:

    if (pointers[IFBOX] == 1 or pointers[IFBOX] == 2 or pointers[IFBOX] == 3)
    {
        /** Rectangular box, dimensions read from the crd file */
        Vector dimensions(crd_box[0], crd_box[1], crd_box[2]);

        // qDebug() << "We have a periodic box of dimensions"
        //          << crdBox[0] << crdBox[1] << crdBox[2];

        // PeriodicBox.
        if (crd_box[3] == 90.0 && crd_box[4] == 90.0 && crd_box[5] == 90.0)
        {
            spce = PeriodicBox(dimensions);
        }
        // TriclinicBox.
        else
        {
            spce = TriclinicBox(dimensions.x(), dimensions.y(), dimensions.z(), crd_box[3] * degrees,
                                crd_box[4] * degrees, crd_box[5] * degrees);
        }
        // spce = PeriodicBox( Vector ( crdBox[0], crdBox[1], crdBox[2] ) ).asA<Space>() ;
        // qDebug() << " periodic box " << spce.toString() ;
    }
    else
    {
        /** Default is a non periodic system */
        spce = Cartesian();
    }
lohedges commented 3 months ago

Hmm, weirdly later in the CI output I see:

Found gemmi version 0.6.6

So perhaps everything is actually okay?

lohedges commented 3 months ago

Okay, everything passing apart from Windows:

2024-07-25T15:10:29.4461850Z FAILED tests/convert/test_rdkit.py::test_rdkit[C1CCCCC1] - TypeError: No to_python (by-value) converter found fo>
2024-07-25T15:10:29.4463410Z FAILED tests/convert/test_rdkit.py::test_rdkit[C] - TypeError: No to_python (by-value) converter found for C++ t>
2024-07-25T15:10:29.4465194Z FAILED tests/convert/test_rdkit.py::test_rdkit[OCC(O)C(O)C(O)C(O)CO] - TypeError: No to_python (by-value) conver>
2024-07-25T15:10:29.4466809Z FAILED tests/convert/test_rdkit.py::test_rdkit[C[C@H](N)C(=O)O] - TypeError: No to_python (by-value) converter f>
2024-07-25T15:10:29.4468531Z FAILED tests/convert/test_rdkit.py::test_rdkit[C[C@@H](N)C(=O)O] - TypeError: No to_python (by-value) converter >
2024-07-25T15:10:29.4470131Z FAILED tests/convert/test_rdkit.py::test_chirality - TypeError: No to_python (by-value) converter found for C++ >
2024-07-25T15:10:29.4471626Z FAILED tests/convert/test_rdkit.py::test_hybridization - TypeError: No to_python (by-value) converter found for >
2024-07-25T15:10:29.4473136Z FAILED tests/convert/test_rdkit.py::test_hybridization2 - TypeError: No to_python (by-value) converter found for>
2024-07-25T15:10:29.4474659Z FAILED tests/convert/test_rdkit.py::test_rdkit_returns_null - TypeError: No to_python (by-value) converter found>
2024-07-25T15:10:29.4476680Z FAILED tests/convert/test_rdkit.py::test_rdkit_infer_bonds - ImportError: You need to install rdkit to be able t>
2024-07-25T15:10:29.4478740Z FAILED tests/convert/test_rdkit.py::test_rdkit_preserve_info - TypeError: No to_python (by-value) converter foun>
2024-07-25T15:10:29.4480251Z FAILED tests/convert/test_smarts.py::test_smarts - TypeError: No to_python (by-value) converter found for C++ ty>
2024-07-25T15:10:29.4483742Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint[atomidx 1538 or atomidx 1518 or atomidx 15>
2024-07-25T15:10:29.4489388Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint[atomidx 1538 or atomidx 1518 or atomidx 15>
2024-07-25T15:10:29.4494570Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint[atomidx 1538 or atomidx 1518 or atomidx 15>
2024-07-25T15:10:29.4499708Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint[atomidx 1538 or atomidx 1518 or atomidx 15>
2024-07-25T15:10:29.4505156Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518-atomidx>
2024-07-25T15:10:29.4510316Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518 or atom>
2024-07-25T15:10:29.4515569Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518 or atom>
2024-07-25T15:10:29.4520796Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518 or atom>
2024-07-25T15:10:29.4526121Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518 or atom>
2024-07-25T15:10:29.4531386Z ERROR tests/restraints/test_boresch.py::test_create_boresch_restraint_fails[atomidx 1538 or atomidx 1518 or atom>
2024-07-25T15:10:29.4536064Z ERROR tests/restraints/test_boresch.py::test_boresch_restraint_params - OSError: SireError::io_error: Cannot loa>
2024-07-25T15:10:29.4540713Z ERROR tests/restraints/test_boresch.py::test_boresch_restraint_params_unstable[atomidx 1538 or atomidx 1518 or a>
2024-07-25T15:10:29.4546015Z ERROR tests/restraints/test_boresch.py::test_boresch_restraint_params_unstable[atomidx 1538 or atomidx 1518 or a>
2024-07-25T15:10:29.4553267Z ERROR tests/restraints/test_boresch.py::test_boresch_restraint_params_unstable[atomidx 1538 or atomidx 1518 or a>
2024-07-25T15:10:29.4558176Z ERROR tests/restraints/test_boresch.py::test_boresch_creation_with_map - OSError: SireError::io_error: Cannot lo>
2024-07-25T15:10:29.4562269Z ERROR tests/restraints/test_standard_state_correction.py::test_standard_state_correction_boresch - OSError: Sire>

@chryswoods: I recall that there was some weirdness with rdkit-dev which was the reason why you needed to put it as a build rather than host requirement. If I put librdkit-dev as a build requirement then it doesn't work for Linux and macOS. However, it seems that putting it in the host section doesn't work for Windows, although it did report that RDKit was correctly found in the CI logs, i.e.:

2024-07-25T13:51:08.1152997Z -- Found RDKit libraries at C:/Users/runneradmin/miniconda3/envs/sire_build/conda-bld/sire_1721912694361/_h_env/>
2024-07-25T13:51:08.1162450Z -- Found RDKit library files at C:/Users/runneradmin/miniconda3/envs/sire_build/conda-bld/sire_1721912694361/_h_>
2024-07-25T13:51:08.1171151Z -- Found RDKit: C:/Users/runneradmin/miniconda3/envs/sire_build/conda-bld/sire_1721912694361/_h_env/Library/incl>
2024-07-25T13:51:08.1172463Z -- Compiling SireRDKit converter

(I'm guessing the Boresch restraints also fail because they are using RDKit internally.)

lohedges commented 3 months ago

And rdkit was definitely installed for both the build and test:

build::

2024-07-25T13:10:58.5407636Z     librdkit:                      2024.03.5-h4c5bbc8_1           conda-forge
2024-07-25T13:10:58.5408828Z     librdkit-dev:                  2024.03.5-h36c15f3_1           conda-forge
2024-07-25T13:16:33.7974170Z     rdkit:                         2024.03.5-py312h9d9823f_1      conda-forge

test:

2024-07-25T15:07:44.9229795Z     rdkit:                     2024.03.5-py312h9d9823f_1     conda-forge

The versions should clearly be compatible.

lohedges commented 3 months ago

Looking at the previous logs (where Windows worked) it seems that RDKit wasn't found during the build, so maybe the tests passed because they were actually skipped somehow.

lohedges commented 3 months ago

The Boresch errors are unrelated to the RDKit changes and appear to be caused by and IO error loading the fixtures for the tests. Hopefully those will be fixed by a simple re-run.

lohedges commented 3 months ago

Yes, the latest run solved the Boresch issues. Now we just have the RDKit issues on Windows:

2024-07-25T16:01:55.4991029Z FAILED tests/convert/test_rdkit.py::test_rdkit[C1CCCCC1] - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.4992681Z FAILED tests/convert/test_rdkit.py::test_rdkit[C] - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.4994274Z FAILED tests/convert/test_rdkit.py::test_rdkit[OCC(O)C(O)C(O)C(O)CO] - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.4995861Z FAILED tests/convert/test_rdkit.py::test_rdkit[C[C@H](N)C(=O)O] - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.4997426Z FAILED tests/convert/test_rdkit.py::test_rdkit[C[C@@H](N)C(=O)O] - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.4998942Z FAILED tests/convert/test_rdkit.py::test_chirality - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.5000435Z FAILED tests/convert/test_rdkit.py::test_hybridization - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.5001943Z FAILED tests/convert/test_rdkit.py::test_hybridization2 - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.5003474Z FAILED tests/convert/test_rdkit.py::test_rdkit_returns_null - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.5005504Z FAILED tests/convert/test_rdkit.py::test_rdkit_infer_bonds - ImportError: You need to install rdkit to be able to generate smiles strings Do this by typing, e.g. 'conda install -c conda-forge rdkit' and then restarting Python and running this script/note>
2024-07-25T16:01:55.5007502Z FAILED tests/convert/test_rdkit.py::test_rdkit_preserve_info - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>
2024-07-25T16:01:55.5009012Z FAILED tests/convert/test_smarts.py::test_smarts - TypeError: No to_python (by-value) converter found for C++ type: class boost::shared_ptr<class RDKit::ROMol>

As mentioned above, I'm not sure we had actually be previously compiling the RDKit support on Windows, so this could be a longstanding issue that we've only just discovered. Not sure how to debug this without access to a Windows box or VM.

chryswoods commented 3 months ago

RDKit support did work on Windows, so it is a regression to lose it. I will look to debug next week.

lohedges commented 3 months ago

Thanks, I thought that was the case but recall it being a pain to get it working.