Minimize api calls in listdir_withattributes - #126
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the listdir_withattributes method to reduce API calls by directly using the FirecREST list_files API instead of making separate calls for each file entry (listdir, get_attribute, and isdir). This performance improvement reduces N API calls to a single call where N is the number of files in the directory.
Changes:
- Added
_parse_permissionshelper to convert permission strings to st_mode integers - Added
_parse_timestamphelper to convert ISO timestamp strings to Unix timestamps - Refactored
listdir_withattributes_asyncto use a singlelist_filesAPI call and parse results directly - Added comprehensive test coverage for
listdir_withattributesfunctionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| aiida_firecrest/transport.py | Added helper methods for parsing file metadata and optimized listdir_withattributes_async to minimize API calls by using list_files directly |
| tests/test_transport.py | Added comprehensive test for listdir_withattributes covering directories, files, symlinks, attributes validation, and pattern filtering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, char in enumerate(perms): | ||
| group_idx = i // 3 | ||
| perm_idx = i % 3 | ||
| if char != "-" and group_idx < 3 and perm_idx < 3: | ||
| mode |= perm_bits[group_idx][perm_idx] |
There was a problem hiding this comment.
The permission parsing logic doesn't handle special permission bits (setuid, setgid, sticky bit). These are typically represented by characters like 's', 'S', 't', 'T' in the permission string (e.g., 'rwsr-xr-x' for setuid). The current implementation will set permission bits for any non-'-' character, which could lead to incorrect permission values. Consider adding explicit handling for these special characters or validating that only 'r', 'w', 'x' are processed.
| (_local / "file1").write_text("content") | ||
| transport.putfile(_local / "file1", _remote / "file1") | ||
| transport.chmod(_remote / "file1", 0o644) | ||
| transport.symlink(_remote / "file1", _remote / "link1") |
There was a problem hiding this comment.
The test verifies that 'link1' appears in the results but doesn't validate the symlink's attributes or isdir flag. Consider adding assertions to verify that results_dict['link1']['isdir'] is False and that stat.S_ISLNK(results_dict['link1']['attributes'].st_mode) is True to ensure symlinks are correctly identified and their attributes properly set.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
+ Coverage 82.03% 84.41% +2.38%
==========================================
Files 4 4
Lines 846 879 +33
==========================================
+ Hits 694 742 +48
+ Misses 152 137 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
No description provided.