feat: ensure FileBasedVariantLookup is used as a context manager #71
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 96.63% 96.65% +0.01%
==========================================
Files 26 26
Lines 1696 1706 +10
Branches 330 331 +1
==========================================
+ Hits 1639 1649 +10
Misses 31 31
Partials 26 26 ☔ View full report in Codecov by Sentry. |
.github/workflows/tests.yml
Outdated
| @@ -35,7 +35,6 @@ jobs: | |||
| - name: Set up miniconda | |||
| uses: conda-incubator/setup-miniconda@v3 | |||
| with: | |||
| miniforge-variant: Mambaforge | |||
There was a problem hiding this comment.
This is necessary now, since support has dropped fully:
Also implemented in:
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces enhancements to the Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
tests/api/test_variant_lookup.py (2)
449-457: LGTM! Consider using a context manager for consistency.This test function effectively checks the logging behavior of
VariantOverlapDetector. The test correctly verifies that the expected debug message is logged when querying a non-existent region.For consistency with the
FileBasedVariantLookuptest, consider using a context manager forVariantOverlapDetectorif it supports it. This would ensure proper resource management and maintain a consistent style across similar tests.
Line range hint
480-507: LGTM! Good test design with randomization and proper resource management.This test function effectively checks the logging behavior when a chromosome is missing from one of the VCFs. The use of a context manager for
FileBasedVariantLookupensures proper resource management. The shuffling of VCF paths is an excellent practice to ensure the test is not order-dependent and is more robust.Regarding the existing comment about moving assertions outside the context manager:
with FileBasedVariantLookup( vcf_paths=vcf_paths, min_maf=0.00, include_missing_mafs=True ) as variant_lookup: variants_of_interest = variant_lookup.query( refname="chr2", start=7999, end=9900 ) assert len(variants_of_interest) == 14 expected_error_msg = "does not contain chromosome" assert expected_error_msg in caplog.textThe suggestion to move the assertions outside the context manager is valid and can be implemented. This change would not affect the test's functionality and might slightly improve readability. However, the current implementation is also correct and doesn't pose any issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/tests.yml (0 hunks)
- prymer/api/variant_lookup.py (4 hunks)
- tests/api/test_variant_lookup.py (5 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🔇 Additional comments (3)
tests/api/test_variant_lookup.py (3)
437-447: LGTM! Good use of context manager and logging capture.This test function effectively checks the logging behavior of
FileBasedVariantLookup. The use of a context manager forFileBasedVariantLookupensures proper resource management, which is a good practice. The test correctly verifies that the expected debug message is logged when querying a non-existent region.
Line range hint
460-467: LGTM! Good error handling and resource management.This test function effectively checks the error handling for missing index files in both
disk_basedandcachedmethods. The use ofpytest.raisesto check for expected exceptions is appropriate. Theclosingcontext manager fordisk_basedensures proper resource management, which is a good practice.
470-477: LGTM! Consistent error handling and resource management.This test function effectively checks the error handling for empty VCF paths in both
disk_basedandcachedmethods. The use ofpytest.raisesto check for expected exceptions is appropriate. Theclosingcontext manager fordisk_basedensures proper resource management, maintaining consistency with the previous test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
prymer/api/variant_lookup.py (1)
325-328: Consider adding a comment for the empty close() methodThe
close()method is currently empty. If this is intentional (e.g., to be overridden by subclasses), consider adding a comment to explain this design choice.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 327-327: prymer/api/variant_lookup.py#L327
Added line #L327 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prymer/api/variant_lookup.py (6 hunks)
- tests/api/test_variant_lookup.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/api/test_variant_lookup.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
prymer/api/variant_lookup.py
[warning] 327-327: prymer/api/variant_lookup.py#L327
Added line #L327 was not covered by tests
🔇 Additional comments (5)
prymer/api/variant_lookup.py (5)
62-62: LGTM: Import changes look goodThe new imports (
AbstractContextManager,TracebackType, andoverride) are appropriate for implementing context management and type checking. These additions align well with the changes made in the classes.Also applies to: 68-68, 79-79
239-239: LGTM: Context management added to VariantLookupThe addition of
AbstractContextManagerto the base class is a good design choice, allowing all subclasses to be used as context managers.
342-354: LGTM: Updated docstring with context management exampleThe updated docstring with an example using context management is helpful and clearly demonstrates the new feature.
480-491: LGTM: Updated docstring with context management exampleThe updated docstring for the
disk_basedfunction includes a clear example of using theFileBasedVariantLookupas a context manager, which is consistent with the changes made to the class.
384-389: LGTM: Proper implementation of close() methodThe
close()method correctly closes all file handles, ensuring proper resource management.To ensure test coverage for the new
close()method, run the following script:✅ Verification successful
Verified:
close()method is properly tested via context managers
- Tests in
tests/api/test_variant_lookup.pyutilizeFileBasedVariantLookupwithinwithstatements, ensuring thatclose()is invoked correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for FileBasedVariantLookup.close() method # Test: Search for test cases covering the close() method rg --type python -g 'test_*.py' 'def test.*close.*FileBasedVariantLookup'Length of output: 106
Script:
#!/bin/bash # Description: Check test coverage for FileBasedVariantLookup.close() method # Test: Search for test cases covering the close() method rg -t py -g 'test_*.py' 'def test.*close.*FileBasedVariantLookup'Length of output: 67
Script:
#!/bin/bash # Description: Check for usage of FileBasedVariantLookup.close() in test files # Test: Search for 'FileBasedVariantLookup.close()' usage in test_*.py files rg -t py -g 'test_*.py' 'FileBasedVariantLookup\.close\('Length of output: 59
Script:
#!/bin/bash # Description: Search for any usage of FileBasedVariantLookup in test files # Test: Find all instances of FileBasedVariantLookup in test_*.py files rg -t py -g 'test_*.py' 'FileBasedVariantLookup'Length of output: 456
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/api/test_variant_lookup.py (2)
448-455: Consider consolidating duplicate test logic.Both test functions share similar setup and assertions. Consider using a parameterized test to reduce code duplication.
-def test_simple_variant_conversion_logs_file_based( - vcf_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - caplog.set_level(logging.DEBUG) - with FileBasedVariantLookup( - vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False - ) as variant_lookup: - variant_lookup.query(refname="foo", start=1, end=2) - assert "No variants extracted from region of interest" in caplog.text - -def test_simple_variant_conversion_logs_non_file_based( - vcf_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - caplog.set_level(logging.DEBUG) - variant_lookup = VariantOverlapDetector( - vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False - ) - variant_lookup.query(refname="foo", start=1, end=2) - assert "No variants extracted from region of interest" in caplog.text +@pytest.mark.parametrize( + "lookup_class,context_manager", + [ + (FileBasedVariantLookup, True), + (VariantOverlapDetector, False), + ], +) +def test_simple_variant_conversion_logs( + vcf_path: Path, + caplog: pytest.LogCaptureFixture, + lookup_class: Type, + context_manager: bool, +) -> None: + caplog.set_level(logging.DEBUG) + kwargs = {"vcf_paths": [vcf_path], "min_maf": 0.01, "include_missing_mafs": False} + + if context_manager: + with lookup_class(**kwargs) as variant_lookup: + variant_lookup.query(refname="foo", start=1, end=2) + else: + variant_lookup = lookup_class(**kwargs) + variant_lookup.query(refname="foo", start=1, end=2) + + assert "No variants extracted from region of interest" in caplog.text
601-613: Improve code comment clarity.The inline comment about "common-mixed-2/2" could be clearer about the VCF format specifics.
- end=9100, # while "common-mixed-2/2" starts at 9101, in the VCf is starts at 9100 + end=9100, # Note: In VCF format, "common-mixed-2/2" is part of a multi-allelic variant at position 9100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
prymer/api/variant_lookup.py(4 hunks)tests/api/test_picking.py(1 hunks)tests/api/test_variant_lookup.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/api/test_picking.py
🔇 Additional comments (6)
prymer/api/variant_lookup.py (3)
67-68: LGTM: Import statements are appropriate.
The added imports for TracebackType and ContextManager are necessary for implementing the context manager protocol.
368-371: LGTM: Resource cleanup is properly implemented.
The close() method correctly closes all underlying VCF file handles.
330-338: LGTM: Documentation is clear and comprehensive.
The updated docstrings provide clear examples of using the context manager functionality, which will help users understand the proper usage pattern.
Also applies to: 474-485
tests/api/test_variant_lookup.py (3)
436-446: LGTM: Clean test split for file-based variant lookup.
The test function properly uses the context manager pattern and has clear assertions.
463-464: LGTM: Proper context manager usage in error tests.
The error handling tests correctly use context managers and maintain the same assertions.
Also applies to: 472-473
494-499: Consider moving assertions outside context manager.
As suggested in the past review comments, consider moving assertions outside the context manager scope for better readability and to follow the test's Arrange-Act-Assert pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prymer/api/variant_lookup.py (1)
364-368: Remove redundantreturn Nonestatement in__exit__methodExplicitly returning
Noneat the end of the__exit__method is unnecessary because Python functions returnNoneby default if no return statement is present.Apply this diff to simplify the method:
def __exit__( self, exc_type: Optional[type[BaseException]], exc_value: Optional[BaseException], traceback: Optional[TracebackType], ) -> None: """Exit this context manager while closing the underlying VCF handles.""" self.close() - return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
prymer/api/variant_lookup.py(5 hunks)
🔇 Additional comments (5)
prymer/api/variant_lookup.py (5)
67-68: Imports added support context management functionality
The addition of TracebackType and ContextManager imports is appropriate and necessary for implementing the context management protocol in FileBasedVariantLookup.
325-325: Class inheritance is appropriate for context management
Inheriting from ContextManager clearly indicates that FileBasedVariantLookup implements the context management protocol, enhancing readability and type hinting.
356-369: Context manager methods are correctly implemented
The __enter__ and __exit__ methods are properly defined, allowing FileBasedVariantLookup instances to be used with the with statement for safer resource management.
382-385: close method correctly releases resources
The close method properly iterates over all VCF file handles and closes them, ensuring that file descriptors are released and preventing potential resource leaks.
477-488: Docstring example is updated for context management
The example in the disk_based function's docstring demonstrates the correct usage of the context manager, which enhances clarity for users on how to safely handle resources.
The
FileBasedVariantLookupopens one or morepysam.VariantFileobjects but provides no public API for closing them. The file-based lookup should have a public method for closing the file handles and should also support use as a context manager so there are safe ways to cleanup IO resources.Closes: #27