test(msa): add MSAManager unit tests (#178)#220
Conversation
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new pytest suite that tests MSA utilities: argument hashing, MSAManager cache-dir handling and cache hit/miss behavior, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/test_msa.py (1)
90-175: Add at least one black-box, no-mock path forMSAManager.get_msaMost tests here are implementation-coupled (private methods + patched internals). Keeping these unit tests is fine, but add one public-interface test that pre-seeds cache files and calls
get_msawithout patching internals.As per coding guidelines:
**/test_*.py: Write black-box tests that verify behavior, not implementation. Use realistic inputs and avoid mocks. Test public interfaces with expected behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_msa.py` around lines 90 - 175, Add a black-box test that exercises the public MSAManager.get_msa without mocking internals: create a realistic input dict and compute the expected hash_key the manager would use, pre-seed the msa cache files (both CSV and A3M as used by _compute_msa's output naming) under tmp_path using that hash_key, instantiate MSAManager (or use existing manager fixture) pointing to tmp_path, call manager.get_msa(data, pairing, structure_predictor=...) and assert the returned mapping points to the pre-seeded files and that manager._cache_hits increments, avoiding any patch.object on _compute_msa or run_mmseqs2 so the test verifies the public behavior only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/utils/test_msa.py`:
- Around line 149-155: The unpaired call assertions are missing checks that auth
and environment flags are forwarded like the paired call; update the assertions
for unpaired_call in tests/utils/test_msa.py to assert that unpaired_call.kwargs
contains the same auth_headers and env-related flags as the paired call (e.g.,
assert unpaired_call.kwargs["auth_headers"] == <expected_auth_headers> and
assert unpaired_call.kwargs["env"] == <expected_env_flags> or equivalent keys
used elsewhere), mirroring the paired-call assertions so unpaired forwarding
cannot regress silently; locate the test by the symbol unpaired_call and add
matching assertions for the auth and env keys used in the existing paired-call
checks.
---
Nitpick comments:
In `@tests/utils/test_msa.py`:
- Around line 90-175: Add a black-box test that exercises the public
MSAManager.get_msa without mocking internals: create a realistic input dict and
compute the expected hash_key the manager would use, pre-seed the msa cache
files (both CSV and A3M as used by _compute_msa's output naming) under tmp_path
using that hash_key, instantiate MSAManager (or use existing manager fixture)
pointing to tmp_path, call manager.get_msa(data, pairing,
structure_predictor=...) and assert the returned mapping points to the
pre-seeded files and that manager._cache_hits increments, avoiding any
patch.object on _compute_msa or run_mmseqs2 so the test verifies the public
behavior only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Closes #178. Covers hash determinism/sensitivity, default vs explicit cache directory handling, get_msa cache hit/miss wiring, run_mmseqs2 argument forwarding (paired and unpaired branches), and csv/a3m file content consistency. All mocks patch the names as imported into sampleworks.utils.msa so the module-level call sites are intercepted.
Mirror the paired-call assertions on the unpaired call so forwarding of use_env and auth_headers can't regress silently.
k-chrispens
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
k-chrispens
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
k-chrispens
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
k-chrispens
left a comment
k-chrispens
left a comment
There was a problem hiding this comment.
This looks correct to me, though I don't see where the original issue said to not add Protenix branch related tests. I believe the tests requested in the issue behave similarly for either branch, so will approve here (differs just what server is called and how things are linked up for Protenix).
2 participants
Summary
Adds tests/utils/test_msa.py covering the five tests requested in #178:
_hash_argumentsis deterministic for identical inputs and sensitive to sequence content, pairing strategy, and value order.msa_cache_diris used (bothPathandstrforms); defaultNonecreates~/.sampleworks/msa(withPath.homemonkeypatched totmp_pathso the real home is untouched).get_msacalls_compute_msawhen cache files are missing, and skips it on the second call when they exist. Cache-hit/api-call counters are asserted._compute_msaforwards the expected arguments torun_mmseqs2for both the paired and unpaired branches (2-sequence input so both fire), includingauth_headersbuilt fromapi_key_header/api_key_value._compute_msawrites matching.csvand.a3mfiles: the second column of the csv equals the odd (sequence) lines of the a3m.Notes
sampleworks.utils.msa.run_mmseqs2andsampleworks.utils.msa._compute_msa(not the source modules) so the actual call sites inmsa.pyare intercepted —run_mmseqs2is imported at module load, and_compute_msais module-level.side_effectthat writes a valid csv/a3m pair so the unconditional_validate_msa_cache_contentscall insideget_msapasses, doubling as a smoke check that the cache layout_compute_msawrites matches whatget_msaexpects.PROTENIX_AVAILABLE.Test plan
pixi run -e boltz-dev python3 -m pytest tests/utils/test_msa.py -v→ 7 passed in 0.03spixi run -e boltz-dev all-tests→ 748 passed, 509 skipped, no regressionsSummary by CodeRabbit