Move DatasetExport into its own dataset_export.py module - #1631
Merged
Conversation
Relocate the generic DatasetExport base out of image_dataset_export.py into a new dataset_export.py. image_dataset_export.py keeps the public ImageDatasetExport and image_sample_to_image (so lightly_studio.export.image_dataset_export still imports) and now imports DatasetExport from the new module. Pure move, no behavior change. The only follow-on edit is retargeting the COCOObjectDetectionOutput writer patch in the test to the module it now lives in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a shared ChangesDataset export pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ImageDatasetExport
participant DatasetExport
participant LabelformatInput
participant LabelformatOutput
ImageDatasetExport->>DatasetExport: initialize with image mapping strategy
DatasetExport->>LabelformatInput: construct format-specific input
DatasetExport->>LabelformatOutput: serialize selected samples and annotations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This reverts commit 7b75dcd.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lightly_studio/src/lightly_studio/export/dataset_export.py`:
- Around line 70-71: Update the Raises documentation for the method accepting
annotation_collection_id so it refers to the annotation collection ID rather
than a nonexistent name, and accurately states the condition under which
ValueError is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17c1114a-f62f-473f-b3c5-59473ca161db
📒 Files selected for processing (3)
lightly_studio/src/lightly_studio/export/dataset_export.pylightly_studio/src/lightly_studio/export/image_dataset_export.pylightly_studio/tests/export/test_image_dataset_export.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lightly_studio/src/lightly_studio/export/dataset_export.py`:
- Around line 20-23: Remove the circular dependency between dataset_export.py
and image_dataset_export.py by relocating the shared constants
DEFAULT_EXPORT_FILENAME, YOLO_DATASET_CONFIG_FILENAME, and YOLO_DEFAULT_SPLIT
into dataset_export.py or a neutral constants module. Update both export modules
to import these values from their new owner while preserving existing behavior
and public references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70b51fc7-5c7f-49de-a721-9dfa4b957723
📒 Files selected for processing (2)
lightly_studio/src/lightly_studio/export/dataset_export.pylightly_studio/src/lightly_studio/export/image_dataset_export.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lightly_studio/src/lightly_studio/export/image_dataset_export.py
Move the export-filename constants into dataset_export.py (their only user is DatasetExport) and drop the back-import from image_dataset_export.py, resolving the module-initialization cycle. Remove the stale ValueError Raises entry from to_coco_object_detections (the method has no such parameter and raises no ValueError). Retarget the writer patch in the test to dataset_export. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What has changed and why
Move DatasetExport to its own file. Done via VSCode Refactor->move to new file + manual import/docstring updates.
Next steps:
How was it tested
existing unittests.