FOEPD-2654 storage unit tests #6646
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughAdds a public class Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
fiftyone/core/storage.py(2 hunks)tests/unittests/storage_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
fiftyone/core/storage.py
196-196: Missing return type annotation for special method __enter__
(ANN204)
199-199: Missing return type annotation for special method __exit__
(ANN204)
tests/unittests/storage_tests.py
88-88: Unnecessary mode argument
Remove mode argument
(UP015)
147-147: Unnecessary mode argument
Remove mode argument
(UP015)
182-182: Unnecessary mode argument
Remove mode argument
(UP015)
290-290: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
349-349: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
432-432: Unnecessary mode argument
Remove mode argument
(UP015)
459-459: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
593-593: Unnecessary mode argument
Remove mode argument
(UP015)
766-766: Unnecessary mode argument
Remove mode argument
(UP015)
820-823: pytest.raises() block should contain a single simple statement
(PT012)
820-820: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
823-823: Avoid specifying long messages outside the exception class
(TRY003)
869-869: Unnecessary mode argument
Remove mode argument
(UP015)
970-970: Unnecessary mode argument
Remove mode argument
(UP015)
975-975: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
979-979: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
987-987: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
1065-1065: Missing return type annotation for private function collect
Add return type annotation: None
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/core/storage.py (1)
222-223: LGTM! Clean implementation of the context manager pattern.The modification correctly wraps the file list in
FileCollection, maintaining backward compatibility while adding context manager support for automatic cleanup.tests/unittests/storage_tests.py (2)
798-871: Excellent test coverage for the new FileCollection context manager.The tests comprehensively validate:
- Context manager protocol functionality
- File handle cleanup on normal exit
- File handle cleanup on exception
- Backward compatibility (FileCollection extends list)
- Both read and write modes
1-1102: Well-structured and comprehensive test suite.The test organization is excellent with:
- Clear class-based grouping by functionality
- Proper fixture usage with cleanup
- Descriptive test names and docstrings
- Coverage of edge cases (skip_failures, nonexistent files, etc.)
Note on static analysis hints: The Ruff warnings flagged are mostly style preferences (e.g., explicit
mode="r"arguments, pytest.raises specificity) and don't represent functional issues. Addressing them is optional.
swheaton
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/storage_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
tests/unittests/storage_tests.py
83-83: Unnecessary mode argument
Remove mode argument
(UP015)
142-142: Unnecessary mode argument
Remove mode argument
(UP015)
177-177: Unnecessary mode argument
Remove mode argument
(UP015)
285-285: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
344-344: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
427-427: Unnecessary mode argument
Remove mode argument
(UP015)
454-454: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
588-588: Unnecessary mode argument
Remove mode argument
(UP015)
761-761: Unnecessary mode argument
Remove mode argument
(UP015)
815-818: pytest.raises() block should contain a single simple statement
(PT012)
815-815: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
818-818: Avoid specifying long messages outside the exception class
(TRY003)
864-864: Unnecessary mode argument
Remove mode argument
(UP015)
965-965: Unnecessary mode argument
Remove mode argument
(UP015)
970-970: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
974-974: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
982-982: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
1060-1060: Missing return type annotation for private function collect
Add return type annotation: None
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-app
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (1)
tests/unittests/storage_tests.py (1)
784-867:open_files/FileCollectiontests look correct and cover key behaviorsThe
TestOpenFilessuite exercises the important behaviors introduced byFileCollection:
- Context-manager usage and collective closing on normal exit (
test_open_files_as_context_manager,test_open_files_closes_files_on_exit)- Proper cleanup on exceptions (
test_open_files_closes_files_on_exception)- Backwards compatibility with list semantics when not used as a context manager (
test_open_files_returns_list)- Write-mode usage via the context manager (
test_open_files_write_mode)These tests align well with the new API contract and should catch most regressions around resource management and type compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
fiftyone/core/storage.py(2 hunks)tests/unittests/storage_tests.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-10-08T15:36:21.356Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-10-08T15:36:21.356Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Applied to files:
tests/unittests/storage_tests.py
📚 Learning: 2024-09-20T14:09:31.848Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Applied to files:
tests/unittests/storage_tests.py
📚 Learning: 2025-12-08T18:35:14.842Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 6646
File: fiftyone/core/storage.py:193-204
Timestamp: 2025-12-08T18:35:14.842Z
Learning: The `fiftyone.core` submodule does not use type annotations.
Applied to files:
fiftyone/core/storage.py
📚 Learning: 2025-04-17T13:48:02.864Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 5642
File: fiftyone/core/collections.py:3910-3957
Timestamp: 2025-04-17T13:48:02.864Z
Learning: Type hints are intentionally not used in the `fiftyone.core` submodule for consistency; suggestions to add them should be avoided unless applied consistently to the entire submodule.
Applied to files:
fiftyone/core/storage.py
🧬 Code graph analysis (1)
tests/unittests/storage_tests.py (5)
fiftyone/core/storage.py (31)
abspath(324-334)join(286-296)isabs(312-321)copy_dir(759-785)sep(274-283)FileSystem(34-37)get_file_system(40-49)get_bucket_name(73-91)get_glob_root(704-731)is_local(94-103)list_available_file_systems(680-687)list_buckets(663-677)copy_file(734-741)move_file(788-795)open_files(208-225)write_file(260-271)read_file(228-238)normalize_path(119-129)open_file(170-190)realpath(299-309)read_json(487-500)load_json(467-484)read_ndjson(536-547)load_ndjson(516-533)write_json(503-513)write_ndjson(550-559)write_yaml(575-584)read_yaml(562-572)split_prefix(52-70)TempDir(150-167)_to_bytes(1027-1032)app/packages/spotlight/src/index.ts (1)
base(189-193)fiftyone/core/sample.py (1)
filename(89-91)fiftyone/operators/executor.py (1)
results(848-850)fiftyone/core/map/mapper.py (1)
num_workers(105-107)
🪛 Ruff (0.14.7)
tests/unittests/storage_tests.py
83-83: Unnecessary mode argument
Remove mode argument
(UP015)
142-142: Unnecessary mode argument
Remove mode argument
(UP015)
177-177: Unnecessary mode argument
Remove mode argument
(UP015)
285-285: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
344-344: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
427-427: Unnecessary mode argument
Remove mode argument
(UP015)
588-588: Unnecessary mode argument
Remove mode argument
(UP015)
761-761: Unnecessary mode argument
Remove mode argument
(UP015)
815-818: pytest.raises() block should contain a single simple statement
(PT012)
818-818: Avoid specifying long messages outside the exception class
(TRY003)
864-864: Unnecessary mode argument
Remove mode argument
(UP015)
965-965: Unnecessary mode argument
Remove mode argument
(UP015)
1068-1068: Missing return type annotation for private function collect
Add return type annotation: None
(ANN202)
fiftyone/core/storage.py
196-196: Missing return type annotation for special method __enter__
(ANN204)
199-199: Missing return type annotation for special method __exit__
(ANN204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: lint / eslint
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (5)
fiftyone/core/storage.py (1)
224-225: Good backwards-compatible enhancement.Wrapping the file list in
FileCollectionenables context manager usage while preserving list semantics.tests/unittests/storage_tests.py (4)
614-625: Good coverage of skip_failures behavior.The test correctly verifies that
open_filesreturns a list containingNonefor failed operations whenskip_failures=True.
807-823: Excellent test of cleanup on exception.This correctly verifies that
FileCollection.__exit__closes all files even when an exception is raised within the context manager.
1083-1111: Well-structured multiworker test.The test correctly verifies that
run()creates a pool with the recommended worker count and exercises theimappath.
1-1112: Comprehensive test coverage achieving 91% as reported.The test suite is well-organized, covers edge cases thoroughly, and correctly validates the new
FileCollectioncontext manager behavior. Good work on the extensive coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fiftyone/core/storage.py (1)
208-225: Update docstring foropen_files()to document the return type and context-manager behaviorThe function now returns
FileCollection(a list subclass with context-manager support), but the docstring still describes it as "a list of open file-like objects" without mentioning the new return type or context-manager capability. Update theReturnssection to reflect the actual behavior:- Returns: - a list of open file-like objects + Returns: + a :class:`FileCollection` (list subclass) of open file-like objects + that can also be used as a context manager to close all filesThe change is backward compatible since
FileCollectionis a list subclass, and no call sites rely on stricttype(...) is listchecks. Tests confirm the context-manager usage is already working correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
fiftyone/core/storage.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-08T18:35:14.842Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 6646
File: fiftyone/core/storage.py:193-204
Timestamp: 2025-12-08T18:35:14.842Z
Learning: The `fiftyone.core` submodule does not use type annotations.
Applied to files:
fiftyone/core/storage.py
📚 Learning: 2025-04-17T13:48:02.864Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 5642
File: fiftyone/core/collections.py:3910-3957
Timestamp: 2025-04-17T13:48:02.864Z
Learning: Type hints are intentionally not used in the `fiftyone.core` submodule for consistency; suggestions to add them should be avoided unless applied consistently to the entire submodule.
Applied to files:
fiftyone/core/storage.py
📚 Learning: 2024-09-20T14:09:31.848Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Applied to files:
fiftyone/core/storage.py
🪛 Ruff (0.14.7)
fiftyone/core/storage.py
196-196: Missing return type annotation for special method __enter__
(ANN204)
199-199: Missing return type annotation for special method __exit__
(ANN204)
203-204: try-except-pass detected, consider logging the exception
(S110)
203-203: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: build / build
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-app
- GitHub Check: build
j053y
left a comment
j053y
left a comment
There was a problem hiding this comment.
🔩 Minor comment but looks good to me once the windows tests pass.
Great base we can build on. Always love to see more automated tests.
tests/unittests/storage_tests.py
Outdated
| @pytest.fixture | ||
| def temp_dir(self): | ||
| tmpdir = tempfile.mkdtemp(prefix="fiftyone_test_") | ||
| yield tmpdir | ||
| shutil.rmtree(tmpdir, ignore_errors=True) |
There was a problem hiding this comment.
Seems like you're redefining this a lot. Might make sense to define at module level once and use in each each test class.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/unittests/storage_tests.py (2)
437-437: Escape regex special characters inpytest.raisesmatch string.The
matchparameter interprets the string as a regex pattern. Ifdir_pathcontains special characters (.,*,[, etc.), the match will fail unexpectedly. Usere.escape()to match the path literally.+import re + with pytest.raises(ValueError, match=re.escape(dir_path) + r" not empty"): fos.ensure_empty_dir(dir_path)
861-865: Remove duplicatetemp_dirfixture; use module-level definition.The
temp_dirfixture is already defined at the module level (lines 27-31). This redefinition shadows it and causes duplication. Delete lines 861-865 and rely on the module-level fixture inherited by all test classes.- @pytest.fixture - def temp_dir(self): - tmpdir = tempfile.mkdtemp(prefix="fiftyone_test_") - yield tmpdir - shutil.rmtree(tmpdir, ignore_errors=True) -
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/storage_tests.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-08-12T17:41:41.645Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Applied to files:
tests/unittests/storage_tests.py
📚 Learning: 2024-09-20T14:09:31.848Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Applied to files:
tests/unittests/storage_tests.py
🧬 Code graph analysis (1)
tests/unittests/storage_tests.py (1)
fiftyone/core/storage.py (38)
abspath(324-334)join(286-296)extract_archive(411-433)make_archive(394-408)copy_files(744-756)read_files(241-257)copy_dir(759-785)sep(274-283)FileSystem(34-37)get_file_system(40-49)get_glob_root(704-731)is_local(94-103)list_available_file_systems(680-687)list_buckets(663-677)copy_file(734-741)delete_file(839-848)ensure_basedir(449-455)isdir(380-391)ensure_dir(458-464)isfile(368-377)list_files(587-636)list_subdirs(648-660)make_temp_dir(132-147)open_files(208-225)read_file(228-238)normalize_path(119-129)normpath(337-353)open_file(170-190)realpath(299-309)read_json(487-500)load_json(467-484)read_ndjson(536-547)write_ndjson(550-559)write_yaml(575-584)read_yaml(562-572)TempDir(150-167)_to_bytes(1027-1032)run(879-900)
🪛 Ruff (0.14.8)
tests/unittests/storage_tests.py
84-84: Unnecessary mode argument
Remove mode argument
(UP015)
137-137: Unnecessary mode argument
Remove mode argument
(UP015)
172-172: Unnecessary mode argument
Remove mode argument
(UP015)
274-274: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
333-333: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
410-410: Unnecessary mode argument
Remove mode argument
(UP015)
571-571: Unnecessary mode argument
Remove mode argument
(UP015)
732-732: Unnecessary mode argument
Remove mode argument
(UP015)
780-783: pytest.raises() block should contain a single simple statement
(PT012)
783-783: Avoid specifying long messages outside the exception class
(TRY003)
829-829: Unnecessary mode argument
Remove mode argument
(UP015)
924-924: Unnecessary mode argument
Remove mode argument
(UP015)
947-947: f-string without any placeholders
Remove extraneous f prefix
(F541)
1025-1025: Missing return type annotation for private function collect
Add return type annotation: None
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build / build
- GitHub Check: test / test-app
- GitHub Check: build / changes
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (1)
tests/unittests/storage_tests.py (1)
755-831: Approve comprehensive context-manager and backward-compatibility tests foropen_files().The test class
TestOpenFilesthoroughly validates the newFileCollectioncontext manager semantics while ensuring list compatibility. Tests cover file closure on normal exit (lines 789-802), exception handling (lines 772-787), list interface backward compatibility (lines 804-818), and write mode (lines 820-830). This aligns well with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fiftyone/core/storage.py (1)
210-224: Alignopen_files()docstring withFileCollectionreturn type and skip-failure behavior
open_files()now returns aFileCollection(alistsubclass) and may containNoneentries whenskip_failuresis true, but the docstring still says “a list of open file-like objects”.Consider tightening the return docs to describe the current API more accurately:
Returns: - a list of open file-like objects + a :class:`FileCollection` of open file-like objects + + Note: + When ``skip_failures`` is True, entries corresponding to failed + opens are ``None``.
♻️ Duplicate comments (1)
tests/unittests/storage_tests.py (1)
861-866: Avoid redefiningtemp_dirfixture insideTestSerializationLocalYou already provide a module-level
temp_dirfixture at Lines 27–31. The class-level fixture here duplicates that behavior and can be dropped for consistency and less boilerplate:- @pytest.fixture - def temp_dir(self): - tmpdir = tempfile.mkdtemp(prefix="fiftyone_test_") - yield tmpdir - shutil.rmtree(tmpdir, ignore_errors=True)Then use the shared
temp_dirfixture in this class as you do elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
fiftyone/core/storage.py(2 hunks)tests/unittests/storage_tests.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-08T18:35:14.879Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 6646
File: fiftyone/core/storage.py:193-204
Timestamp: 2025-12-08T18:35:14.879Z
Learning: The `fiftyone.core` submodule does not use type annotations.
Applied to files:
fiftyone/core/storage.py
📚 Learning: 2025-04-17T13:48:02.864Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 5642
File: fiftyone/core/collections.py:3910-3957
Timestamp: 2025-04-17T13:48:02.864Z
Learning: Type hints are intentionally not used in the `fiftyone.core` submodule for consistency; suggestions to add them should be avoided unless applied consistently to the entire submodule.
Applied to files:
fiftyone/core/storage.py
📚 Learning: 2025-12-09T15:35:45.409Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 6646
File: tests/unittests/storage_tests.py:84-84
Timestamp: 2025-12-09T15:35:45.409Z
Learning: In the voxel51/fiftyone repository, when reviewing Python code that uses open(), do not suggest removing explicit mode="r" arguments even if Ruff flags UP015. The maintainers prefer explicit open mode for readability. Apply this guideline to all Python files in the repo (tests and source) where open() is used, not just a single file.
Applied to files:
fiftyone/core/storage.pytests/unittests/storage_tests.py
📚 Learning: 2024-08-12T17:41:41.645Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 4651
File: fiftyone/__public__.py:116-116
Timestamp: 2024-08-12T17:41:41.645Z
Learning: In `fiftyone/__public__.py`, imports are intended to make names available to users at the top module level, even if they are not used directly in the file. Avoid suggesting the removal of unused imports in this file.
Applied to files:
tests/unittests/storage_tests.py
📚 Learning: 2024-09-20T14:09:31.848Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 0
File: :0-0
Timestamp: 2024-09-20T14:09:31.848Z
Learning: Ensure that code review comments on `fiftyone/core/cli.py` are accurate and relevant to the actual code changes.
Applied to files:
tests/unittests/storage_tests.py
📚 Learning: 2025-12-09T15:36:23.193Z
Learnt from: swheaton
Repo: voxel51/fiftyone PR: 6646
File: tests/unittests/storage_tests.py:274-274
Timestamp: 2025-12-09T15:36:23.193Z
Learning: In pytest.mark.parametrize, when parametrizing a single parameter you can use a plain string (e.g., 'param_name') as a shorthand. The tuple syntax ('param_name',) is only required when you are parametrizing multiple parameters. Do not flag PT006 for single-parameter parametrizations; use the string shorthand for clarity and correctness across tests.
Applied to files:
tests/unittests/storage_tests.py
🪛 Ruff (0.14.8)
fiftyone/core/storage.py
196-196: Missing return type annotation for special method __enter__
(ANN204)
199-199: Missing return type annotation for special method __exit__
(ANN204)
203-203: Do not catch blind exception: Exception
(BLE001)
tests/unittests/storage_tests.py
84-84: Unnecessary mode argument
Remove mode argument
(UP015)
137-137: Unnecessary mode argument
Remove mode argument
(UP015)
172-172: Unnecessary mode argument
Remove mode argument
(UP015)
274-274: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
333-333: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
410-410: Unnecessary mode argument
Remove mode argument
(UP015)
437-437: f-string without any placeholders
Remove extraneous f prefix
(F541)
571-571: Unnecessary mode argument
Remove mode argument
(UP015)
732-732: Unnecessary mode argument
Remove mode argument
(UP015)
780-783: pytest.raises() block should contain a single simple statement
(PT012)
783-783: Avoid specifying long messages outside the exception class
(TRY003)
829-829: Unnecessary mode argument
Remove mode argument
(UP015)
924-924: Unnecessary mode argument
Remove mode argument
(UP015)
1025-1025: Missing return type annotation for private function collect
Add return type annotation: None
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: build
🔇 Additional comments (2)
tests/unittests/storage_tests.py (2)
755-828: Strong coverage ofopen_files()/FileCollectionbehaviorThese tests (context manager usage, exception path, normal exit, non-context usage, and write-mode handling) exercise the key semantics of the new
FileCollectionwrapper and its backward compatibility as alist. No changes needed here.
1040-1068:run()multiworker test correctly validates Pool construction and usageThe multiworker test now verifies that
fou.recommend_thread_pool_workers()feeds through tomultiprocessing.dummy.Pool(processes=...)and that theimappath is exercised with the expected results. This pins down the parallel branch behavior well without over-specifying internals.
| class FileCollection(list): | ||
| """A list of open file-like objects with a closing context manager""" | ||
|
|
||
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| for f in self: | ||
| try: | ||
| f.close() | ||
| except Exception: # catch all so we continue to close the rest | ||
| logger.debug( | ||
| "FileCollection failed to close %r", f, exc_info=True | ||
| ) | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Skip closing sentinel None entries in FileCollection.__exit__
When open_files(..., skip_failures=True) is used, failed opens produce None entries (as your tests document). __exit__ currently calls close() on every element and logs at DEBUG on any exception, which guarantees a debug log for each None and relies on AttributeError from the sentinel.
You can avoid this noise and make the intent explicit by skipping None while preserving existing behavior for real file-like objects:
def __exit__(self, exc_type, exc_val, exc_tb):
- for f in self:
- try:
- f.close()
- except Exception: # catch all so we continue to close the rest
- logger.debug(
- "FileCollection failed to close %r", f, exc_info=True
- )
+ for f in self:
+ if f is None:
+ continue
+
+ try:
+ f.close()
+ except Exception: # catch all so we continue to close the rest
+ logger.debug(
+ "FileCollection failed to close %r", f, exc_info=True
+ )
return False🧰 Tools
🪛 Ruff (0.14.8)
196-196: Missing return type annotation for special method __enter__
(ANN204)
199-199: Missing return type annotation for special method __exit__
(ANN204)
203-203: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In fiftyone/core/storage.py around lines 193 to 207, update
FileCollection.__exit__ so it skips sentinel None entries instead of attempting
to call close() on them; specifically, in the for-loop check "if f is None:
continue" (or equivalent) before calling f.close(), keep the existing try/except
around close() and the logger.debug call for real exceptions, and return False
as before so behavior for real file-like objects and error handling remains
unchanged.
|
@CodeRabbit ignore |
✅ Actions performedReviews paused. |
What changes are proposed in this pull request?
open_files()return a context manager so that all files can be closed. Since it extendslist, it should be completely backwards compatible.How is this patch tested? If it is not, please explain why.
Added unit test coverage to 91%
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.