-
Notifications
You must be signed in to change notification settings - Fork 310
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
[wip] Pandas Dataframe in Dataclass #3116
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #fedbf7Actionable Suggestions - 2
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
import pandas as pd | ||
if isinstance(python_val, pd.DataFrame): | ||
python_val = StructuredDataset(dataframe=python_val, file_format="parquet") |
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.
Consider moving the pandas DataFrame conversion logic to a dedicated transformer class instead of handling it in the _make_dataclass_serializable
method. This would improve code organization and maintainability.
Code suggestion
Check the AI-generated fix before applying
import pandas as pd | |
if isinstance(python_val, pd.DataFrame): | |
python_val = StructuredDataset(dataframe=python_val, file_format="parquet") |
Code Review Run #fedbf7
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
decoder = JSONDecoder(new_expected_python_type) | ||
self._json_decoder[new_expected_python_type] = decoder |
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.
Consider storing the decoder in the original expected_python_type
key instead of new_expected_python_type
to avoid potential memory leaks from storing multiple decoders for the same logical type.
Code suggestion
Check the AI-generated fix before applying
decoder = JSONDecoder(new_expected_python_type) | |
self._json_decoder[new_expected_python_type] = decoder | |
decoder = JSONDecoder(new_expected_python_type) | |
self._json_decoder[expected_python_type] = decoder |
Code Review Run #fedbf7
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #6cea0dActionable Suggestions - 0Review Details
|
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #b16268Actionable Suggestions - 2
Review Details
|
from flytekit.types.structured.structured_dataset import StructuredDataset | ||
from typing import get_type_hints, Type, Dict | ||
|
||
def convert_dataclass(instance, target_cls): |
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.
The convert_dataclass
function could benefit from type hints for better code maintainability and IDE support. Consider adding type annotations for instance
and target_cls
parameters.
Code suggestion
Check the AI-generated fix before applying
def convert_dataclass(instance, target_cls): | |
def convert_dataclass(instance: Any, target_cls: Type[T]) -> T: |
Code Review Run #b16268
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
||
dc = decoder.decode(json_str) | ||
|
||
return self._fix_dataclass_int(expected_python_type, dc) | ||
return convert_dataclass(self._fix_dataclass_int(new_expected_python_type, dc), expected_python_type) |
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.
The conversion from new_expected_python_type
to expected_python_type
may lose data if the types have different field structures. Consider validating field compatibility before conversion.
Code suggestion
Check the AI-generated fix before applying
return convert_dataclass(self._fix_dataclass_int(new_expected_python_type, dc), expected_python_type) | |
fixed_dc = self._fix_dataclass_int(new_expected_python_type, dc) | |
# Validate field compatibility | |
if not all(f.name in [ef.name for ef in fields(expected_python_type)] for f in fields(fixed_dc.__class__)): | |
raise ValueError(f"Incompatible field structure between {new_expected_python_type.__name__} and {expected_python_type.__name__}") | |
return convert_dataclass(fixed_dc, expected_python_type) |
Code Review Run #b16268
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #9c627aActionable Suggestions - 1
Review Details
|
flytekit/core/type_engine.py
Outdated
def transform_dataclass(cls, memo=None): | ||
if memo is None: | ||
memo = {} | ||
|
||
if cls in memo: | ||
return memo[cls] | ||
|
||
cls_hints = get_type_hints(cls) | ||
new_field_defs = [] | ||
for field in fields(cls): | ||
orig_type = cls_hints[field.name] | ||
if orig_type == pd.DataFrame: | ||
new_type = StructuredDataset | ||
elif is_dataclass(orig_type): | ||
new_type = transform_dataclass(orig_type, memo) | ||
else: | ||
new_type = orig_type | ||
new_field_defs.append((field.name, new_type)) | ||
|
||
new_cls = make_dataclass("FlyteModified" + cls.__name__, new_field_defs) | ||
memo[cls] = new_cls | ||
return new_cls |
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.
Consider extracting the transform_dataclass
function to avoid code duplication. This function appears multiple times in the codebase (lines 689-710, 764-785, and 1014-1035) with identical functionality.
Code suggestion
Check the AI-generated fix before applying
def transform_dataclass(cls, memo=None): | |
if memo is None: | |
memo = {} | |
if cls in memo: | |
return memo[cls] | |
cls_hints = get_type_hints(cls) | |
new_field_defs = [] | |
for field in fields(cls): | |
orig_type = cls_hints[field.name] | |
if orig_type == pd.DataFrame: | |
new_type = StructuredDataset | |
elif is_dataclass(orig_type): | |
new_type = transform_dataclass(orig_type, memo) | |
else: | |
new_type = orig_type | |
new_field_defs.append((field.name, new_type)) | |
new_cls = make_dataclass("FlyteModified" + cls.__name__, new_field_defs) | |
memo[cls] = new_cls | |
return new_cls | |
from flytekit.core.utils import transform_dataclass |
Code Review Run #9c627a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3116 +/- ##
===========================================
- Coverage 76.85% 41.34% -35.52%
===========================================
Files 206 206
Lines 21851 21955 +104
Branches 2837 2860 +23
===========================================
- Hits 16794 9077 -7717
- Misses 4269 12729 +8460
+ Partials 788 149 -639 ☔ View full report in Codecov by Sentry. |
Code Review Agent Run #e17dbeActionable Suggestions - 3
Review Details
|
FLYTE_TYPES = [FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema] | ||
if cls in FLYTE_TYPES: | ||
return cls |
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.
Consider moving the FLYTE_TYPES
list to a module-level constant since it's used in multiple places within the same file. This would improve maintainability and reduce duplication.
Code suggestion
Check the AI-generated fix before applying
FLYTE_TYPES = [FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema] | |
if cls in FLYTE_TYPES: | |
return cls | |
FLYTE_TYPES = [FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema] | |
if cls in FLYTE_TYPES: return cls |
Code Review Run #e17dbe
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
FLYTE_TYPES = [FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema] | ||
if cls in FLYTE_TYPES: | ||
return cls |
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.
Consider using a tuple or frozenset instead of list for FLYTE_TYPES
since it appears to be a constant collection that won't be modified. This provides better performance for lookups and makes the intent clearer.
Code suggestion
Check the AI-generated fix before applying
FLYTE_TYPES = [FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema] | |
if cls in FLYTE_TYPES: | |
return cls | |
FLYTE_TYPES = (FlyteFile, FlyteDirectory, StructuredDataset, FlyteSchema) | |
if cls in FLYTE_TYPES: | |
return cls |
Code Review Run #e17dbe
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #c4bd83Actionable Suggestions - 1
Review Details
|
@@ -702,7 +737,7 @@ def to_generic_literal( | |||
encoder = self._json_encoder[python_type] | |||
except KeyError: | |||
encoder = JSONEncoder(python_type) | |||
self._json_encoder[python_type] = encoder | |||
self._json_encoder[new_python_type] = encoder |
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.
Consider using python_type
instead of new_python_type
when storing the encoder in self._json_encoder
. The encoder is created using python_type
but stored with new_python_type
, which could lead to inconsistencies in encoder lookup.
Code suggestion
Check the AI-generated fix before applying
self._json_encoder[new_python_type] = encoder | |
self._json_encoder[python_type] = encoder |
Code Review Run #c4bd83
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #8760deActionable Suggestions - 0Additional Suggestions - 10
Review Details
|
Tracking issue
Why are the changes needed?
we want to improve UX.
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR enhances Flytekit's functionality in multiple areas: 1) Improves the DataclassTransformer with automatic Pandas DataFrame to StructuredDataset conversion and better type handling, 2) Enhances error handling and validation for msgpack data, authentication, and secret mount management, 3) Implements recursive transformation capabilities and improved JSON decoding, 4) Introduces configurable caching policies and strengthens type validation features.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5