Return StructuredDataset which is a field in a dataclass #3071
+77
−0
Conversation
This file contains 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
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
arbaobao
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #63793cActionable Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Jan 21, 2025
Comment on lines
+738
to
+742
| if isinstance(python_val._literal_sd, StructuredDataset): | ||
| sdt = StructuredDatasetType(format=python_val._literal_sd.file_format) | ||
| metad = literals.StructuredDatasetMetadata(structured_dataset_type=sdt) | ||
| sd_literal = literals.StructuredDataset(uri=python_val._literal_sd.uri, metadata=metad) | ||
| return Literal(scalar=Scalar(structured_dataset=sd_literal)) |
There was a problem hiding this comment.
Private member access needs encapsulation
Accessing private member '_literal_sd'. Consider using a public interface or property to access this data.
Code suggestion
Check the AI-generated fix before applying
- if literal_type.structured_dataset_type is not None and self._literal_sd is not None:
- return self._literal_sd
- if literal_type.structured_dataset_type is not None and self._literal_sd is None:
+ if literal_type.structured_dataset_type is not None and self.literal_sd is not None:
+ return self.literal_sd
+ if literal_type.structured_dataset_type is not None and self.literal_sd is None:
Code Review Run #63793c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flyte-bot
reviewed
Jan 21, 2025
Comment on lines
+738
to
+742
| if isinstance(python_val._literal_sd, StructuredDataset): | ||
| sdt = StructuredDatasetType(format=python_val._literal_sd.file_format) | ||
| metad = literals.StructuredDatasetMetadata(structured_dataset_type=sdt) | ||
| sd_literal = literals.StructuredDataset(uri=python_val._literal_sd.uri, metadata=metad) | ||
| return Literal(scalar=Scalar(structured_dataset=sd_literal)) |
There was a problem hiding this comment.
Consider extracting literal creation logic
The code block for handling StructuredDataset passed through dataclass could be simplified by extracting the literal creation logic into a helper method. This would improve code readability and maintainability.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| if isinstance(python_val._literal_sd, StructuredDataset): | |
| sdt = StructuredDatasetType(format=python_val._literal_sd.file_format) | |
| metad = literals.StructuredDatasetMetadata(structured_dataset_type=sdt) | |
| sd_literal = literals.StructuredDataset(uri=python_val._literal_sd.uri, metadata=metad) | |
| return Literal(scalar=Scalar(structured_dataset=sd_literal)) | |
| if isinstance(python_val._literal_sd, StructuredDataset): | |
| return self._create_structured_dataset_literal(python_val._literal_sd.uri, python_val._literal_sd.file_format) | |
| def _create_structured_dataset_literal(self, uri: str, file_format: str) -> Literal: | |
| sdt = StructuredDatasetType(format=file_format) | |
| metad = literals.StructuredDatasetMetadata(structured_dataset_type=sdt) | |
| sd_literal = literals.StructuredDataset(uri=uri, metadata=metad) | |
| return Literal(scalar=Scalar(structured_dataset=sd_literal)) |
Code Review Run #63793c
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Nelson Chen <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3071 +/- ##
==========================================
- Coverage 80.13% 79.64% -0.50%
==========================================
Files 272 202 -70
Lines 24614 21479 -3135
Branches 2768 2769 +1
==========================================
- Hits 19725 17106 -2619
+ Misses 4082 3604 -478
+ Partials 807 769 -38 ☔ View full report in Codecov by Sentry. |
|
When we return a StructuredDataset attribute from a dataclass instance, an error occurs : This issue has been addressed in this PR. |
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Code Review Agent Run #d93af6Actionable Suggestions - 1
Additional Suggestions - 10
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 4, 2025
| @task | ||
| def read_sd(dc: DC) -> StructuredDataset: | ||
| """Read input StructuredDataset.""" | ||
| print("sd:", dc.sd.open(pd.DataFrame).all()) |
There was a problem hiding this comment.
Consider adding error handling for dataset access
Consider adding error handling around open() and all() calls to handle potential exceptions when accessing the structured dataset.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| print("sd:", dc.sd.open(pd.DataFrame).all()) | |
| try: | |
| df = dc.sd.open(pd.DataFrame).all() | |
| print("sd:", df) | |
| except Exception as e: | |
| print(f"Error accessing structured dataset: {e}") | |
| raise |
Code Review Run #d93af6
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tracking issue
Related to #6117
Why are the changes needed?
If we wrap the StructuredDataset in a dataclass, it will fail during the to_flyte_idl conversion.
What changes were proposed in this pull request?
Before returning
Literals, we check the type ofpython_val._literal_sd. If it is a Python nativeStructuredDataset, we transform it into aLiterals.StructuredDataset.How was this patch tested?
As described in #6117, an error occurs when the
extracttask is executed.Setup process
Screenshots
Check all the applicable boxes
Summary by Bito
This PR fixes StructuredDataset handling within dataclasses during to_flyte_idl conversion by properly transforming python_val._literal_sd instances into Literals.StructuredDataset. It also introduces a new Kubernetes StatefulSet Data Service plugin, enhances image specification with custom Python executable support, and implements configurable chunk sizes for S3/GCS operations. The changes include improved resource management and enhanced Ray plugin configuration capabilities.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5