Skip to content

RDKit 2026.3 support and minor bug fix#172

Merged
scal444 merged 4 commits into
NVIDIA-Digital-Bio:mainfrom
scal444:more_bugfixes
May 13, 2026
Merged

RDKit 2026.3 support and minor bug fix#172
scal444 merged 4 commits into
NVIDIA-Digital-Bio:mainfrom
scal444:more_bugfixes

Conversation

@scal444
Copy link
Copy Markdown
Collaborator

@scal444 scal444 commented May 13, 2026

  • Removed system from boost targets. It's been header only since 1.67, so the target isn't needed as long as the include dir is right. Boost 1.90 (rdkit links to this) dropped system from their targets so it was a compile issue.

  • Fixed a stream ordering bug in MMFF. It was latent, I think our recent constraint code caused it to be hit, but did not cause the issue directly.

  • Track RDKit version to match results for multiple bug fixes.

  • Remove flaky memory test. Due to memory pooling/caching, it was never reliably correct.

scal444 added 4 commits May 13, 2026 08:45
ONe of the assertions in the memory free test was incorrect,
due to memory caching, one can potentially allocate device memory
without an exact corresponding increase in observed memory usage.

STream order bug was latent, I think recent changes to constraints
have exposed it, but it predates v0.5
@scal444 scal444 requested a review from evasnow1992 May 13, 2026 14:53
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 13, 2026

Greptile Summary

This PR adds RDKit 2026.03 compatibility by version-gating two behavioral changes (gradient scaling sign convention and TFD beta-bond typo) against compile-time RDKit version macros, fixes a CUDA stream ordering bug in MMFF minimization, drops the boost::system link target (header-only since Boost 1.67, removed in Boost 1.90), and removes a flaky memory test.

  • Stream ordering fix (bfgs_mmff.cpp): setStreams is now called before sendContribsAndIndicesToDevice, ensuring device operations use the correct CUDA stream.
  • Version-conditioned behavior: kRdkitHasGradScaleFix (RDKit \u2265 2025.09) switches gradient comparison to fabs, and kRdkitHasBetaTypoFix (RDKit \u2265 2026.03) corrects the duplicate-nb2 check in calculateBeta; both use a new nvmolkit_versions INTERFACE CMake target to distribute versions.h.
  • Build fix: boost::system removed from BOOST_TARGET_LIBS; flaky test_async_gpu_result_release_frees_memory removed.

Confidence Score: 4/5

Safe to merge; the stream-ordering fix and version-gated behavioral guards are correct, and no existing callers are broken.

The kRdkitHasGradScaleFix constant is defined identically in both bfgs_minimize.cu and bfgs_minimize_permol_kernels.cu. If the version cutoff changes, it must be kept in sync across two files rather than one central location in versions.h.

src/minimizer/bfgs_minimize.cu and src/minimizer/bfgs_minimize_permol_kernels.cu share a duplicated version constant that would benefit from centralization.

Important Files Changed

Filename Overview
cmake/boost.cmake Removes system from Boost link targets; correct since Boost.System has been header-only since 1.67 and Boost 1.90 dropped the CMake target entirely.
nvmolkit/tests/test_types.py Removes the flaky test_async_gpu_result_release_frees_memory test that was unreliable due to CUDA memory pooling; remaining tests are unaffected.
src/minimizer/bfgs_mmff.cpp Fixes stream-ordering bug: setStreams now runs before sendContribsAndIndicesToDevice, ensuring all device work uses the correct CUDA stream.
src/minimizer/bfgs_minimize.cu Adds version-conditioned gradient scaling (fabs vs. signed) to match RDKit 2025.09 behavior; kRdkitHasGradScaleFix is duplicated in the permol-kernels file.
src/minimizer/bfgs_minimize_permol_kernels.cu Mirrors the gradient-scaling version guard from bfgs_minimize.cu; the constant is defined twice rather than being centralized in versions.h.
src/tfd/tfd_common.cpp Replaces hardcoded RDKit beta-typo replication with a compile-time version check; correctly reproduces both old and new logic for respective RDKit releases.
src/CMakeLists.txt Introduces nvmolkit_versions INTERFACE library exposing the generated versions.h, threading it into embedder_utils instead of a per-target include_directories.
src/minimizer/CMakeLists.txt Adds nvmolkit_versions to the bfgs private link libraries so version macros are available in minimizer CUDA sources.
src/tfd/CMakeLists.txt Adds nvmolkit_versions to tfd_cpu private link libraries; straightforward dependency addition.

Reviews (1): Last reviewed commit: "Formatting" | Re-trigger Greptile

Comment thread src/minimizer/bfgs_minimize_permol_kernels.cu
Copy link
Copy Markdown
Collaborator

@evasnow1992 evasnow1992 left a comment

Choose a reason for hiding this comment

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

LGTM

@scal444 scal444 merged commit 7322c9c into NVIDIA-Digital-Bio:main May 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants