3D Scene I/O optimization and bugfixes #4442
Conversation
WalkthroughThe updates span across several modules, focusing on enhancing the handling of 3D scene assets, refining path resolution logic, and improving metadata computation. Key changes include refactoring methods to include caching, updating path resolution functions, and adding new test cases to ensure functionality. Additionally, new parameters were introduced to handle 3D scene assets in various dataset operations. Changes
Sequence Diagram(s) (Beta)No sequence diagrams are necessary for these changes. Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- fiftyone/core/threed/object_3d.py (3 hunks)
- fiftyone/core/threed/scene_3d.py (2 hunks)
- tests/unittests/threed/scene_3d_tests.py (1 hunks)
Additional comments not posted (4)
tests/unittests/threed/scene_3d_tests.py (1)
74-102: The new unit testtest_update_asset_pathseffectively checks the functionality of updating asset paths in a scene object. It's good to see that the test covers various types of asset paths, including relative paths and specific file types. This should help ensure that the asset path update logic handles different scenarios correctly.fiftyone/core/threed/object_3d.py (2)
Line range hint
29-40: The change in theObject3Dclass to inherit fromobjectand the simplification of the_asset_path_fieldsto an empty list are both straightforward and align with the PR's objective to simplify asset path management. This should reduce complexity and potential errors in asset path handling.
230-230: The_get_asset_pathsmethod has been simplified due to the empty_asset_path_fields. This change should improve performance slightly by reducing unnecessary attribute checks and method calls.fiftyone/core/threed/scene_3d.py (1)
189-189: The updates to theSceneclass to handle asset path updates more robustly are well-implemented. The methodupdate_asset_pathsnow correctly handles both the scene's assets and the background assets, ensuring that all paths are updated as specified. This is crucial for maintaining consistency and correctness when asset paths are changed.
[APROVED]Also applies to: 279-279, 287-290, 296-296
brimoor
changed the title
|
should this be targeting |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (12)
fiftyone/core/storage.py (3)
Line range hint
8-8: Remove unused import.The
contextlib.contextmanageris imported but not used in this file. Removing unused imports can help reduce the file size and improve load time.- from contextlib import contextmanager
285-286: Clarify the documentation for path resolution.The documentation for the
resolvefunction could be enhanced by specifying what types of symlinks and relative paths are resolved. This would provide clearer guidance to users on how the function behaves.
Line range hint
885-885: Avoid using bareexceptstatements.Similar to the previous comment in the other file, avoid using bare
exceptstatements to improve error handling specificity and maintainability.- except: + except Exception as e: + logger.error(f"Failed to open file due to: {str(e)}") + return Nonefiftyone/utils/data/exporters.py (9)
Line range hint
541-541: Avoid using bareexceptstatements.- except: + except Exception as e:
Line range hint
571-571: Avoid using bareexceptstatements.- except: + except Exception as e:
Line range hint
614-614: Avoid using bareexceptstatements.- except: + except Exception as e:
Line range hint
649-649: Avoid using bareexceptstatements.- except: + except Exception as e:
Line range hint
904-904: Avoid using bareexceptstatements.- except: + except Exception as e:
Line range hint
3815-3815: Use direct truth checks instead of comparing toFalse.- if include_confidence == False: + if not include_confidence:
Line range hint
3815-3815: Use direct truth checks instead of comparing toFalse.- if include_attributes == False: + if not include_attributes:
Line range hint
3941-3941: Use direct truth checks instead of comparing toTrue.- if include_confidence == True: + if include_confidence:
Line range hint
3946-3946: Use direct truth checks instead of comparing toTrue.- if include_attributes == True: + if include_attributes:
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/core/metadata.py (7 hunks)
- fiftyone/core/storage.py (1 hunks)
- fiftyone/core/threed/scene_3d.py (3 hunks)
- fiftyone/utils/data/exporters.py (3 hunks)
- fiftyone/utils/utils3d.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/core/threed/scene_3d.py
Additional context used
Ruff
fiftyone/core/metadata.py
561-561: Do not use bare
exceptfiftyone/core/storage.py
8-8:
contextlib.contextmanagerimported but unused
885-885: Do not use bare
exceptfiftyone/utils/data/exporters.py
541-541: Do not use bare
except
571-571: Do not use bare
except
614-614: Do not use bare
except
649-649: Do not use bare
except
904-904: Do not use bare
except
3815-3815: Avoid inequality comparisons to
False; useif include_confidence:for truth checks
3815-3815: Avoid inequality comparisons to
False; useif include_attributes:for truth checks
3941-3941: Avoid equality comparisons to
True; useif include_confidence:for truth checks
3946-3946: Avoid equality comparisons to
True; useif include_attributes:for truth checks
Additional comments not posted (2)
fiftyone/core/metadata.py (1)
Line range hint
486-527: Verify the effectiveness of the new caching mechanism.fiftyone/utils/utils3d.py (1)
474-474: Simplified asset path resolution enhances readability and potentially improves performance.
fiftyone/utils/utils3d.py
Outdated
| scene_dir = os.path.dirname(original_scene_path) | ||
| for i, asset_path in enumerate(asset_paths): | ||
| if not fos.isabs(asset_path): | ||
| asset_path = fos.join(scene_dir, asset_path) | ||
| asset_paths[i] = fos.resolve(asset_path) | ||
| asset_paths[i] = fos.resolve(fos.join(scene_dir, asset_path)) |
There was a problem hiding this comment.
Optimize asset path resolution logic.
The current implementation resolves the absolute path for each asset sequentially within a loop. This could be inefficient if there are many assets. Consider using vectorized operations or list comprehensions to handle path operations in bulk, which could potentially improve performance.
| @classmethod | ||
| def build_for(cls, scene_path, mime_type=None, _cache=None): | ||
| """Builds a :class:`SceneMetadata` object for the given 3D scene. | ||
|
|
||
| Args: | ||
| scene_path: a scene path | ||
| mime_type (None): the MIME type of the scene. If not provided, | ||
| defaults to ``application/octet-stream`` | ||
|
|
||
| Returns: | ||
| a :class:`SceneMetadata` | ||
| """ | ||
| if scene_path.startswith("http"): | ||
| return cls._build_for_url( | ||
| scene_path, mime_type=mime_type, cache=_cache | ||
| ) | ||
|
|
||
| return cls._build_for_local( | ||
| scene_path, mime_type=mime_type, cache=_cache | ||
| ) | ||
|
|
||
| @classmethod | ||
| def _build_for_local(cls, scene_path, mime_type=None, cache=None): | ||
| if mime_type is None: | ||
| mime_type = "application/octet-stream" | ||
|
|
||
| scene_size = os.path.getsize(scene_path) | ||
| scene = fo3d.Scene.from_fo3d(scene_path) | ||
|
|
||
| asset_counts, asset_size = _parse_assets( | ||
| scene, scene_path, cache=cache | ||
| ) | ||
| size_bytes = scene_size + asset_size | ||
|
|
||
| return cls( | ||
| size_bytes=size_bytes, | ||
| mime_type=mime_type, | ||
| asset_counts=asset_counts, | ||
| ) |
There was a problem hiding this comment.
Refactor to improve caching mechanism.
The caching mechanism can be improved by ensuring that the cache is utilized more efficiently. Consider checking the cache before making file system calls or processing assets, which could save significant processing time, especially for large datasets.
|
|
||
| try: | ||
| return _get_metadata(filepath, media_type) | ||
| return _get_metadata(filepath, media_type, cache=cache) | ||
| except: |
There was a problem hiding this comment.
Avoid using bare except statements.
Using bare except statements can catch unexpected exceptions and make debugging more difficult. Specify the exception type to improve error handling.
- except:
+ except Exception as e:
+ logger.error(f"Failed to get metadata due to: {str(e)}")
+ return NoneCommittable suggestion was skipped due low confidence.
Tools
Ruff
561-561: Do not use bare
except
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/utils/data/exporters.py (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/utils/data/exporters.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- fiftyone/core/metadata.py (7 hunks)
- fiftyone/core/storage.py (3 hunks)
- fiftyone/core/threed/scene_3d.py (3 hunks)
- fiftyone/utils/data/exporters.py (6 hunks)
- fiftyone/utils/utils3d.py (3 hunks)
- tests/unittests/metadata_tests.py (1 hunks)
- tests/unittests/utils3d_tests.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/unittests/utils3d_tests.py
Files skipped from review as they are similar to previous changes (4)
- fiftyone/core/storage.py
- fiftyone/core/threed/scene_3d.py
- fiftyone/utils/data/exporters.py
- fiftyone/utils/utils3d.py
Additional comments not posted (3)
tests/unittests/metadata_tests.py (2)
33-35: Ensure the logic for not counting duplicate assets is correctly implemented in the metadata computation.#!/bin/bash # Description: Verify that duplicate assets are not counted in the metadata computation. # Test: Search for the logic handling duplicate assets in metadata computation. rg --type python $'metadata.asset_counts' --context 5
43-45: Ensure the calculation ofexpected_sizecorrectly includes the size of the scene file and all assets.#!/bin/bash # Description: Verify that the size calculation includes all assets. # Test: Search for the size calculation logic in the metadata computation. rg --type python $'metadata.size_bytes' --context 5fiftyone/core/metadata.py (1)
486-488: Ensure the caching mechanism is correctly implemented and utilized across metadata computation functions.#!/bin/bash # Description: Verify that the caching mechanism is correctly implemented. # Test: Search for the usage of the cache parameter in metadata computation functions. rg --type python $'cache=' --context 5Also applies to: 525-527, 546-548, 554-555
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/utils/data/exporters.py (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/utils/data/exporters.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- tests/unittests/threed/scene_3d_tests.py (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/unittests/threed/scene_3d_tests.py
| def resolve(path): | ||
| """Resolves path to absolute, resolving symlinks and relative path | ||
| indicators such as `.` and `..`. | ||
| def realpath(path): |
There was a problem hiding this comment.
Just noting this is a public API change
There was a problem hiding this comment.
Yes true. My thinking was that this was only introduced in fiftyone==0.24.0 and the spirit of the fiftyone.core.storage module is to exactly match the os package's naming convention for the methods that it wraps, so this felt consistent.
Change log
Optimizations
compute_metadata()to avoid recomputing metadata for repeated 3D asset pathsBugfixes
Summary by CodeRabbit
New Features
include_assetsparameter in variousfo.Datasetmethods to handle 3D scene assets.download_scenesmethod to thefo.Datasetclass.Bug Fixes
SceneMetadataandSceneclasses to ensure correct path resolution and scene modification.Exporterclass to handle different export modes based onself.export_mode.Tests
test_update_asset_pathsmethod to verify asset path updates in scenes.