Validate batchesPerGpu in HardwareOptions rather than individual callers

[volgin]

Summary

• batchesPerGpu validation (reject values ≤ 0 other than -1) previously lived only in EmbedMolecules, leaving MMFFOptimizeMoleculesConfs and any future API unprotected. Invalid values passed to MMFF would reach the C++ layer and produce cryptic errors.
• Move the check into HardwareOptions.__init__ and the batchesPerGpu setter so every consumer is covered consistently.
• Remove the now-redundant check from EmbedMolecules.
• Add parametrized regression test for invalid values (0, -2, -99).

Context

We use nvMolKit for GPU-accelerated conformer generation in a drug discovery pipeline (Novel Therapeutics). We hit this gap when passing hardware options to MMFFOptimizeMoleculesConfs — invalid batchesPerGpu values produced opaque C++ errors instead of a clear Python-side ValueError.

Test plan

• Existing tests pass (validation behavior unchanged for EmbedMolecules)
• New test_embed_molecules_invalid_batches_per_gpu covers 0, -2, -99
• MMFFOptimizeMoleculesConfs now rejects invalid values at HardwareOptions construction

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR moves batchesPerGpu validation from EmbedMolecules into HardwareOptions itself (both __init__ and the batchesPerGpu setter), ensuring all consumers — including MMFFOptimizeMoleculesConfs and any future callers — receive a clear Python-side ValueError for invalid values instead of a cryptic C++ error. The implementation is clean: __init__ now delegates to the setter (single source of truth), the redundant check in embedMolecules.py is removed, and the new parametrized test in test_types.py covers both the constructor and setter code paths.

Key changes:

• HardwareOptions.batchesPerGpu setter now validates that the value is either -1 (auto) or > 0, raising ValueError otherwise
• HardwareOptions.__init__ assigns batchesPerGpu via the setter after self._native is initialised, avoiding duplicated logic
• Redundant validation block removed from EmbedMolecules
• New test_hardware_options_invalid_batches_per_gpu parametrized test covers values 0, -2, and -99 for both construction and setter paths — directly addressing the previously noted gap

Confidence Score: 5/5

• This PR is safe to merge — it consolidates validation into a single location with no behavioural regressions for valid inputs.
• The change is narrowly scoped: validation logic is moved, not altered. The sentinel value (-1) and the positive-integer requirement are preserved exactly. The __init__ correctly assigns self._native before invoking the setter, so there is no ordering hazard. Both previously-flagged thread concerns (setter test coverage and duplicated logic) have been fully addressed. No security or performance concerns.
• No files require special attention.

Important Files Changed

Filename	Overview
nvmolkit/types.py	Moves batchesPerGpu validation into the setter and delegates from init to it, eliminating duplication. The ordering in init (self._native assigned before the setter call) is correct. Logic is clean and safe.
nvmolkit/embedMolecules.py	Removes the now-redundant batchesPerGpu check. The removal is safe because HardwareOptions constructor/setter enforces the same rule earlier, before native_options is extracted.
nvmolkit/tests/test_types.py	Adds a parametrized test covering both construction-time and setter validation paths for invalid batchesPerGpu values. The pytest.raises match pattern is a valid substring of the actual error message and will match correctly.

_{Last reviewed commit: 2ba3e0c}

[evasnow1992]

Since this is now testing HardwareOptions validation rather than EmbedMolecules behavior, could you move this test to test_types.py?

[evasnow1992]

Thanks for the contribution! The validation consolidation looks good. One minor comment: the new test would be better placed in test_types.py since it's testing HardwareOptions directly. Happy to approve once that's moved.

[evasnow1992]

Thanks for moving the test to test_types.py! The validation changes look good. However, could you please remove the .github/workflows/test.yml from this PR? CI workflow configuration is managed separately by the project maintainers. Once that's removed, we're good to merge.

[evasnow1992]

Thank you for your contribution!