Skip to content

Sync with Recursion remote #400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 28, 2025
Merged

Sync with Recursion remote #400

merged 4 commits into from
May 28, 2025

Conversation

lohedges
Copy link
Contributor

This PR synchronises with changes from the Recursion remote.

Implement a hang killer at BSS level
@lohedges lohedges added the recursion Related to work with Recursion label May 27, 2025
@lohedges lohedges temporarily deployed to biosimspace-build May 27, 2025 08:47 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build May 27, 2025 08:47 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 27, 2025 08:47 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build May 27, 2025 08:47 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build May 27, 2025 08:47 — with GitHub Actions Failure
@lohedges
Copy link
Contributor Author

Seeing failed tests on various platforms (unexpected ones, i.e. not related to CI issue or missing Sire packages):

FAILED tests/IO/test_file_cache.py::test_file_cache_mol_nums - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Parameters/test_parameters.py::test_molecule_rename - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Parameters/test_parameters.py::test_acdoctor - Failed: DID NOT RAISE <class 'BioSimSpace._Exceptions._exceptions.ParameterisationError'>
FAILED tests/Sandpit/Exscientia/IO/test_file_cache.py::test_file_cache_mol_nums - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Parameters/test_parameters.py::test_molecule_rename - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Parameters/test_parameters.py::test_acdoctor - Failed: DID NOT RAISE <class 'BioSimSpace.Sandpit.Exscientia._Exceptions._exceptions.ParameterisationError'>
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system_vel-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system_vel-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system_vel0-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Production-system_vel0-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system_vel-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system_vel-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system_vel0-True] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
FAILED tests/Sandpit/Exscientia/Process/test_restart.py::test_process[Equilibration-system_vel0-False] - ValueError: could not convert string to float: 'Need both input and output files and formats.'
...

As always, this isn't reproducible locally. Will see if I can recreate the exact environment.

@lohedges
Copy link
Contributor Author

Having looked back at the CI, these errors also appeared in #398, so they aren't unique to this PR.

@lohedges
Copy link
Contributor Author

Ah, this is an issue with the updated ambertools package since antechamber no longer reports a version so the following code fails:

    version = float(string.split(":")[0])

I'll wrap this in a try/except block.

@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 08:50 — with GitHub Actions Inactive
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 08:50 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 08:50 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 08:50 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 08:50 — with GitHub Actions Inactive
@lohedges
Copy link
Contributor Author

I decided to just remove the version check since openff now automatically pulls in a pinned version of ambertools anyway. The original code was written when we just used the base openff packages and allowed the use of an external ambertools specified via AMBERHOME, which could be incompatible.

@lohedges
Copy link
Contributor Author

lohedges commented May 28, 2025

This has fixed the ValueError exceptions reported above, but I'm still seeing the following error on Linux Python 3.10 and macOS Python 3.11:

   @pytest.mark.skipif(
        has_antechamber is False or has_tleap is False,
        reason="Requires AmberTools/antechamber and tLEaP to be installed.",
    )
    def test_acdoctor():
        """
        Test that parameterising negatively charged molecules works when acdoctor
        is disabled.
        """
    
        # Load the molecule.
        mol = BSS.IO.readMolecules(f"{url}/negative_charge.sdf")[0]
    
        # Make sure parameterisation fails when acdoctor is enabled.
>       with pytest.raises(BSS._Exceptions.ParameterisationError):
E       Failed: DID NOT RAISE <class 'BioSimSpace._Exceptions._exceptions.ParameterisationError'>

I assume that antechamber has been updated so that acdoctor is no longer required. It's annoying that this is platform dependent. It appears that older Python variants have newer versions on ambertools:

Linux Python 3.10 (fails):

ambertools:                       24.8-cuda_None_nompi_py310h1ac786c_100 conda-forge         

Linux Python 3.11 (works):

    ambertools:                       23.6-cuda_None_nompi_py311h4a53416_105 conda-forge         

Not sure if I should just pin ambertools for now (other issues have been reported elsewhere for other dependencies) or just skip the test for now. Annoyingly, as the other error has shown there's no way to determine the version of antechamber so we can't conditionally change the xfail based on the version.

@lohedges
Copy link
Contributor Author

sander does report the version information, so maybe we could use that as a proxy, i.e.:

sander -v
sander: Version 24.0

@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 10:46 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 10:46 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 10:46 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 10:46 — with GitHub Actions Failure
@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 10:46 — with GitHub Actions Failure
@lohedges
Copy link
Contributor Author

Bah, that works locally in a conda environment with ambertools23 and 24, but doesn't work in the CI since sander doesn't report any version info either. I'll just skip the test for now.

@lohedges lohedges had a problem deploying to biosimspace-build May 28, 2025 11:08 — with GitHub Actions Failure
@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 11:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 11:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 11:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build May 28, 2025 11:08 — with GitHub Actions Inactive
@lohedges lohedges merged commit 8ba863f into devel May 28, 2025
4 of 5 checks passed
@lohedges lohedges deleted the sync_recursion2 branch May 28, 2025 12:49
lohedges added a commit that referenced this pull request Jun 12, 2025
lohedges added a commit that referenced this pull request Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recursion Related to work with Recursion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants