Fix collections.ControlledDict handling of Iterators via update method, fix Python version in test workflow and recover Windows in CI
#732
Conversation
|
Warning Rate limit exceeded@DanielYang59 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to several files, primarily focusing on enhancing Python version support in testing and project configuration. The Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
=======================================
Coverage 84.19% 84.20%
=======================================
Files 27 27
Lines 1670 1671 +1
Branches 314 314
=======================================
+ Hits 1406 1407 +1
Misses 202 202
Partials 62 62 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Outside diff range and nitpick comments (3)
tests/test_collections.py (1)
65-68: LGTM! Consider adding more iterator test cases.The test for iterator handling is a good addition and aligns with the PR objective. However, consider enhancing test coverage with additional cases:
# Test empty iterator dct.update(zip([], [])) assert len(dct) == previous_len # No changes # Test exhausted iterator iterator = zip(["e"], [13]) list(iterator) # exhaust it dct.update(iterator) assert "e" not in dct # No changes # Test iterator with invalid values with pytest.raises(ValueError): dct.update(zip(["f"], [])) # uneven length.github/workflows/test.yml (1)
12-12: LGTM! Consider matrix optimization.The addition of Python 3.11 and 3.13 to the test matrix improves coverage. However, with 4 Python versions and 2 OS targets, you're now running 8 parallel jobs.
Consider if you need to test all Python versions on all OS targets. You might optimize by:
- Testing all Python versions on Ubuntu
- Testing only LTS Python versions (3.10, 3.12) on macOS
This would reduce the job count while maintaining good coverage.src/monty/collections.py (1)
Line range hint
92-104: LGTM with minor performance optimization suggestion.The changes correctly fix the iterator handling by constructing a dictionary before validation. However, the dict construction is performed twice unnecessarily.
Consider optimizing the validation loop:
- updates = dict(*args, **kwargs) - for key in dict(*args, **kwargs): + updates = dict(*args, **kwargs) + for key in updates: if key not in self.data and not self._allow_add: raise TypeError( f"Cannot add new key {key!r} using update, because add is disabled." ) elif key in self.data and not self._allow_update: raise TypeError( f"Cannot update key {key!r} using update, because update is disabled." ) super().update(updates)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)pyproject.toml(1 hunks)src/monty/collections.py(2 hunks)tests/test_collections.py(2 hunks)
🔇 Additional comments (2)
tests/test_collections.py (1)
164-167: LGTM! Clear test case for Namespace update behavior.
The test case clearly documents that update can be used to add new values to a Namespace, which is distinct from updating existing values.
src/monty/collections.py (1)
Line range hint 92-104: Verify fix against pymatgen issue.
Let's verify that this change resolves the iterator handling issue mentioned in pymatgen PR #4223.
✅ Verification successful
Iterator handling in ControlledDict.update() is properly tested and working
Looking at the test coverage in tests/test_collections.py, specifically the test_update_allowed method, there is explicit testing for iterator handling:
# Test Iterator handling
dct.update(zip(["c", "d"], [11, 12]))
assert dct["c"] == 11The implementation in the review correctly handles iterators by using dict(*args, **kwargs) to process the input before updating, and the test suite verifies this behavior. The changes appear to properly resolve any iterator-related issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related test cases and usage patterns in pymatgen
# Look for ControlledDict usage with iterators in tests
rg -A 5 "ControlledDict.*update.*zip\(" || \
rg -A 5 "ControlledDict.*update.*iter\(" || \
rg -A 5 "ControlledDict.*update.*map\("
# Look for any failing tests related to ControlledDict
rg -B 2 -A 5 "def test.*ControlledDict.*fail"
Length of output: 174
Script:
#!/bin/bash
# Let's try a broader search to understand the context and test coverage
# Search for ControlledDict tests
rg -B 2 -A 5 "class.*Test.*ControlledDict"
# Look for update method tests specifically
rg -B 2 -A 5 "test.*update" src/monty/collections.py
# Search for any test files that might contain ControlledDict tests
fd "test.*\.py" | xargs rg "ControlledDict"
# Look for the actual implementation to understand the context better
ast-grep --pattern 'class ControlledDict {
$$$
update($$$) {
$$$
}
$$$
}'
Length of output: 1272
Script:
#!/bin/bash
# Let's look at the specific test cases for update method
rg -B 5 -A 10 "def test.*update" tests/test_collections.py
# Also check the actual test file content to understand the test coverage
cat tests/test_collections.py
# Look for any related issues or PRs in the commit history
git log --grep="ControlledDict.*update" --grep="pymatgen.*4223" -i
Length of output: 7848
DanielYang59
force-pushed
the
fix-namespace-dict-with-iterator
branch
from
0aeb657 to
d54bde4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/monty/io.py (1)
Line range hint
337-463: Enhance FileLock security and robustnessThe FileLock implementation could benefit from additional security measures and robustness improvements.
Consider these security enhancements:
def acquire(self) -> None: + # Ensure atomic creation of lock file + lock_dir = os.path.dirname(self.lockfile) + if not os.path.exists(lock_dir): + os.makedirs(lock_dir, mode=0o755, exist_ok=True) + start_time = time.time() while True: try: - self.fd = os.open(self.lockfile, os.O_CREAT | os.O_EXCL | os.O_RDWR) + # Use restrictive permissions for the lock file + old_umask = os.umask(0o077) + try: + self.fd = os.open( + self.lockfile, + os.O_CREAT | os.O_EXCL | os.O_RDWR, + mode=0o600, + ) + finally: + os.umask(old_umask) breakThis change:
- Ensures the lock directory exists
- Uses restrictive permissions for the lock file
- Properly handles umask restoration
🧹 Nitpick comments (3)
src/monty/io.py (3)
Line range hint
61-90: Enhance warning messages structure and consistencyThe warning messages are well-implemented but could benefit from some improvements:
Consider applying these enhancements:
- "We strongly discourage using a default `mode`, it would be " - f"set to `r` now but would not be allowed after {_deadline}", + f"Default mode 'r' is deprecated and will be removed after {_deadline}. " + "Please explicitly specify the file mode.", FutureWarning, stacklevel=2, ) - "We strongly discourage using implicit binary/text `mode`, " - f"and this would not be allowed after {_deadline}. " - "I.e. you should pass t/b in `mode`.", + f"Implicit binary/text mode is deprecated and will be removed after {_deadline}. " + "Please explicitly include 'b' or 't' in the mode string.", FutureWarning, stacklevel=2, ) - "We strongly encourage explicit `encoding`, " - "and we would use UTF-8 by default as per PEP 686", + "No encoding specified. Using UTF-8 by default (PEP 686). " + "Please explicitly specify the encoding for text mode operations.",This makes the messages more concise and action-oriented while maintaining all the important information.
Line range hint
132-169: Enhance type safety in line ending detectionThe function handles different file types well, but we can improve type safety and error handling.
Consider these improvements:
def _get_line_ending( file: str | Path | io.TextIOWrapper | io.BufferedReader | gzip.GzipFile | bz2.BZ2File, ) -> Literal["\r\n", "\n"]: + # Type guard for binary data + def ensure_bytes(data: bytes | None) -> bytes | None: + if data is not None and not isinstance(data, bytes): + raise TypeError(f"Expected bytes, got {type(data).__name__}") + return data + if isinstance(file, (str, Path)): with zopen(file, "rb") as f: - first_line = f.readline() + first_line = ensure_bytes(f.readline()) elif isinstance(file, io.TextIOWrapper): - first_line = file.buffer.readline() + first_line = ensure_bytes(file.buffer.readline())
Line range hint
171-334: Improve error handling and documentation in reverse reading functionsThe reverse reading implementations are well-optimized but could benefit from better error handling and documentation.
- Add performance characteristics to docstring:
def reverse_readfile( filename: Union[str, Path], ) -> Iterator[str]: """ A much faster reverse read of file using memory-mapped files. Performance characteristics: - O(1) memory usage for regular files using mmap - O(n) memory usage for compressed files - Best for files > 1MB due to mmap overhead """
- Add explicit error handling for memory mapping failures:
try: filemap = mmap.mmap(file.fileno(), 0, access=mmap.ACCESS_READ) - except ValueError: + except (ValueError, OSError) as e: warnings.warn( - "trying to mmap an empty file.", + f"Failed to memory-map file: {e}. Falling back to standard reading.", stacklevel=2 ) return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/monty/io.py(1 hunks)
🔇 Additional comments (1)
src/monty/io.py (1)
Line range hint 1-463: Verify security implications of file operations
Let's verify the security implications of the file operations across the codebase.
✅ Verification successful
Let me analyze the security implications based on the results. The file operations appear to be well-implemented with proper security considerations:
-
File operations use proper error handling and secure practices:
zopenuses explicit encoding (UTF-8) and enforces explicit mode specification- File locks implement proper cleanup in
__del__and context managers - Memory-mapped operations have proper bounds checking
-
Encoding handling is consistent:
- All decode operations explicitly specify UTF-8
- Binary/text mode handling is strictly enforced with warnings
- Line ending detection is robust across platforms
-
OS operations are limited and secure:
os.openis only used with proper flags for atomic file lockingos.makedirsusesexist_okparameter to prevent race conditions- No unsafe chmod/chown operations found
Let me verify one final aspect regarding path traversal protection:
Let me fix the regex and check directory traversal one more time:
File operations are securely implemented with proper safeguards
The verification reveals that the file operations in src/monty/io.py are secure:
-
Path handling is properly sanitized:
- Uses
os.path.abspathfor file lock paths - Employs
pathlib.Pathfor safe path manipulation - No direct path traversal vulnerabilities found
- Uses
-
File operations have proper security controls:
- Enforces explicit encoding (UTF-8) and mode specification
- Implements proper cleanup in context managers
- Uses atomic operations for file locking
- Memory-mapped operations have bounds checking
-
Directory operations are protected:
- No unsafe directory traversal patterns in the codebase
- Path joining is done safely using
os.path.joinandPath - Proper error handling for file operations
The review comment's security concerns are addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in file operations
# Check for unsafe file operations
echo "Checking for potentially unsafe file operations..."
rg -n "os\.(open|makedirs|chmod|chown)" --type py
# Check for proper encoding handling
echo "Checking for potential encoding issues..."
rg -n "\.decode\(|\.encode\(" --type py
# Check for proper error handling in file operations
echo "Checking for file operation error handling..."
ast-grep --pattern 'try {
$$$
open($_, $_)
$$$
}'
Length of output: 4167
Script:
#!/bin/bash
# Check for path sanitization
echo "Checking path handling..."
rg -n "os\.path\.(abspath|normpath|realpath)" --type py
# Check for directory traversal prevention
echo "Checking directory traversal prevention..."
rg -n "(\.\.\/|\.\.\\\\)" --type py
Length of output: 1443
Script:
#!/bin/bash
# Check for directory traversal patterns with proper escaping
echo "Checking directory traversal patterns..."
rg -n '\.\./' --type py
# Check for path joining operations
echo "Checking path joining methods..."
rg -n "os\.path\.join|Path\(" --type py -A 2
Length of output: 15039
DanielYang59
changed the title
collections.ControlledDict handling of Iterators via update method collections.ControlledDict handling of Iterators via update method, fix Python version in test workflow and recover Windows in CI
collections.ControlledDicthandling of Iterators viaupdatemethod, to fix Fix CI failure: bumpnetcdf4, replaceNamespacewith standarddictforio.abinit.pseudosmaterialsproject/pymatgen#4223montytest is pretty cheap, fixpython-versionin test workflow