-
Notifications
You must be signed in to change notification settings - Fork 57
chore: move ArtifactStorage to engine/storage/ module #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…eInferenceParameters, EmbeddingInferenceParameters
…th BaseInferenceParameters
…lved based on the type of InferenceParameters
Consolidate all storage logic (ArtifactStorage, MediaStorage) under engine/storage/ for better organization. Update all imports across source and test files.
Greptile OverviewGreptile SummaryThis PR consolidates storage-related code by moving Key changes:
Note: The PR description mentions updating
|
| Filename | Overview |
|---|---|
| packages/data-designer-engine/src/data_designer/engine/storage/artifact_storage.py | File moved from dataset_builders/ to storage/ without content changes |
| packages/data-designer-engine/tests/engine/storage/test_artifact_storage.py | Test file moved to match new location, imports updated correctly, patch path updated |
| packages/data-designer-engine/src/data_designer/engine/dataset_builders/column_wise_builder.py | Import updated and duplicate _has_image_columns method removed |
| packages/data-designer/src/data_designer/integrations/huggingface/client.py | Import path updated to new storage location for constants |
| packages/data-designer/src/data_designer/interface/data_designer.py | Import path updated to new storage location |
Last reviewed commit: f0ea307
- Reduce num_records to 2 for image generation in tutorial notebook - Add tests for different image response formats (dict and plain string) - Parametrize PNG/JPG media storage tests for better maintainability
…pati/chore/move-artifact-storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20 files reviewed, 1 comment
| "MediaStorage", | ||
| "StorageMode", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing newline
packages/data-designer-engine/src/data_designer/engine/storage/__init__.py appears to be missing a final newline at EOF. Some tooling (e.g., strict linters / formatting checks) treats this as a failure. Adding a newline after the closing ] avoids CI/style check issues.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/storage/__init__.py
Line: 23:25
Comment:
**Missing trailing newline**
`packages/data-designer-engine/src/data_designer/engine/storage/__init__.py` appears to be missing a final newline at EOF. Some tooling (e.g., strict linters / formatting checks) treats this as a failure. Adding a newline after the closing `]` avoids CI/style check issues.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.…pati/chore/move-artifact-storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20 files reviewed, 1 comment
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/storage/__init__.py
Line: 6:6
Comment:
PR description states this file should export `ArtifactStorage`, `BatchStage`, and related constants, but they're not in `__all__`. Consider adding them for consistency with the PR description.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
📋 Summary
Consolidate all storage logic under
engine/storage/by movingArtifactStoragealongsideMediaStoragefor better code organization.🔄 Changes
🔧 Changed
artifact_storage.pyfromengine/dataset_builders/toengine/storage/test_artifact_storage.pyfromtests/engine/dataset_builders/totests/engine/storage/engine/storage/__init__.pyto exportArtifactStorage,BatchStage, and related constants#317 needs to merge first