Skip to content
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

Fix issue #265 #266

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Fix issue #265 #266

merged 2 commits into from
Dec 13, 2024

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Dec 9, 2024

This PR closes #265 by changing the way that periodic box vectors are updated in the SOMD OpenMM context. This change is needed due to this issue from OpenMM. We now only need to update the default box vectors in the system, then reinitialise the context, after which the updated vectors will be propagated. Without this, calling setPeriodicBoxVectors() directly on the context will trigger an exception when using certain platforms, e.g. CUDA.

I've added tests for this update in BioSimSpace here. Note that these will always pass in the CI since the CPU platform is unaffected by this issue. As such, the test will need to be run manually on a GPU enabled device in order to validate. (The CUDA platform is conditionally used in the tests when CUDA_VISIBLE_DEVICES is set.)

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Copy link
Contributor

@mb2055 mb2055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good 👍

Just to clarify, does this mean that SOMD is now only compatible with openmm versions 8.2+?

@lohedges
Copy link
Contributor Author

No, this works for any version. The previous logic was unnecessary, but still (thankfully) gives the same result.

@lohedges lohedges merged commit e33ceec into devel Dec 13, 2024
5 checks passed
@lohedges lohedges deleted the fix_265 branch December 13, 2024 14:45
lohedges added a commit that referenced this pull request Dec 13, 2024
@lohedges
Copy link
Contributor Author

lohedges commented Dec 14, 2024

Despite the CI passing for the PR, it's now repeatedly failing for Windows for the devel build. I'll check next week, but presumably some dependency in the environment has changed.

@lohedges
Copy link
Contributor Author

This is the exception raise during compilation of the wrappers on Windows, which worked on the CI only a few days ago:

2024-12-14T09:18:04.9473896Z   Generating Code...
2024-12-14T09:18:04.9475005Z      Creating library C:/Users/runneradmin/miniconda3/envs/sire_build/conda-bld/sire_1734161070384/work/build/conda_build_wrapper/Mol/Release/Mol.lib and object C:/Users/runneradmin/miniconda3/envs/sire_build/conda-bld/sire_1734161070384/work/build/conda>
2024-12-14T09:18:04.9476345Z   LINK : /LTCG specified but no code generation required; remove /LTCG from the link command line to improve linker performance
2024-12-14T09:18:04.9476901Z   Mol.vcxproj -> %SRC_DIR%\build\conda_build_wrapper\Mol\Release\_Mol.pyd
2024-12-14T09:18:04.9477240Z SOMETHING WENT WRONG WHEN COMPILING THE WRAPPERS!

@chryswoods: Any ideas? The LTCG bit looks to be a warning, not an error, so I'm not sure why it failed.

This is the setup for the failing runner:


Current runner version: '2.321.0'
Operating System
  Microsoft Windows Server 2022
  10.0.20348
  Datacenter
Runner Image
  Image: windows-2022
  Version: 20241211.1.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20241211.1/images/windows/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20241211.1
Runner Image Provisioner
  2.0.385.1

The last one that worked was:

Current runner version: '2.321.0'
Operating System
  Microsoft Windows Server 2022
  10.0.20348
  Datacenter
Runner Image
  Image: windows-2022
  Version: 20241201.2.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20241201.2/images/windows/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20241201.2
Runner Image Provisioner
  2.0.385.1

The only difference appears to be the Runner image version, which is 20241211.1.0 for the one that doesn't work and 20241201.2.0 for the one that does.

@lohedges
Copy link
Contributor Author

The LTCG bit is a red herring, since the same message appears in the working build.

lohedges added a commit that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] openmm version 8.2.0 effect SOMD due to box shape identify
2 participants