Attempt to use more specific transformer when executing remote entities #3111
+213
−6
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: Yee Hing Tong <[email protected]>
Code Review Agent Run Status
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3111 +/- ##
==========================================
- Coverage 80.01% 76.11% -3.90%
==========================================
Files 318 204 -114
Lines 27075 21636 -5439
Branches 2779 2792 +13
==========================================
- Hits 21663 16469 -5194
+ Misses 4647 4344 -303
- Partials 765 823 +58 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yee Hing Tong <[email protected]>
wild-endeavor
requested review from
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #e0c471Actionable Suggestions - 1
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Feb 7, 2025
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Code Review Agent Run #2f3b74Actionable Suggestions - 2
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 7, 2025
flyte-bot
reviewed
Feb 7, 2025
Signed-off-by: Yee Hing Tong <[email protected]>
Code Review Agent Run #e0383aActionable Suggestions - 1
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 7, 2025
eapolinario
reviewed
Feb 8, 2025
2 tasks
Signed-off-by: Yee Hing Tong <[email protected]>
Code Review Agent Run #b17d18Actionable Suggestions - 0Review Details
AI Code Review powered by |
Signed-off-by: Yee Hing Tong <[email protected]>
Code Review Agent Run #67094dActionable Suggestions - 2
Additional Suggestions - 1
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 11, 2025
Comment on lines
+1517
to
+1522
| type_hints[k] = strict_type_hint_matching(v, input_flyte_type_map[k].type) | ||
| except ValueError: | ||
| logger.debug(f"Could not guess type for {input_flyte_type_map[k].type}, skipping...") | ||
| developer_logger.debug( | ||
| f"Could not guess type for {input_flyte_type_map[k].type}, skipping..." | ||
| ) | ||
| type_hints[k] = TypeEngine.guess_python_type(input_flyte_type_map[k].type) |
There was a problem hiding this comment.
Consider more specific error handling
Consider handling the ValueError exception more gracefully. Currently, if strict_type_hint_matching() fails, we fall back to TypeEngine.guess_python_type() which may also raise a ValueError. This could lead to confusing error messages for users. Consider adding more specific error handling.
Code suggestion
Check the AI-generated fix before applying
- type_hints[k] = strict_type_hint_matching(v, input_flyte_type_map[k].type)
- except ValueError:
- developer_logger.debug(
- f"Could not guess type for {input_flyte_type_map[k].type}, skipping..."
- )
- type_hints[k] = TypeEngine.guess_python_type(input_flyte_type_map[k].type)
+ type_hints[k] = strict_type_hint_matching(v, input_flyte_type_map[k].type)
+ except ValueError as e:
+ developer_logger.debug(f"strict_type_hint_matching failed: {str(e)}")
+ try:
+ type_hints[k] = TypeEngine.guess_python_type(input_flyte_type_map[k].type)
+ except ValueError as e2:
+ developer_logger.error(
+ f"Failed to guess type for {input_flyte_type_map[k].type}: {str(e2)}"
+ )
+ raise
Code Review Run #67094d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flyte-bot
reviewed
Feb 11, 2025
| @@ -2423,6 +2424,27 @@ def dataclass_from_dict(cls: type, src: typing.Dict[str, typing.Any]) -> typing. | |||
| return cls(**constructor_inputs) | |||
|
|
|||
|
|
|||
| def strict_type_hint_matching(input_val: typing.Any, target_literal_type: LiteralType) -> typing.Type: | |||
There was a problem hiding this comment.
Consider impact of strict type matching
The function name change from better_guess_type_hint to strict_type_hint_matching better reflects its behavior since it raises ValueError on type mismatches rather than falling back to guessing. However, this is a breaking change that could impact existing code relying on the fallback behavior.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| def strict_type_hint_matching(input_val: typing.Any, target_literal_type: LiteralType) -> typing.Type: | |
| def better_guess_type_hint(input_val: typing.Any, target_literal_type: LiteralType) -> typing.Type: | |
| native_type = type(input_val) | |
| try: | |
| return strict_type_hint_matching(input_val, target_literal_type) | |
| except ValueError: | |
| return TypeEngine.guess_python_type(target_literal_type) | |
| def strict_type_hint_matching(input_val: typing.Any, target_literal_type: LiteralType) -> typing.Type: |
Code Review Run #67094d
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
flyteorg/flyte#6156
Why are the changes needed?
Please see issue. When FlyteRemote tries to execute a fetched task, it doesn't have any Python types, it only has the Flyte types. This PR attempts to try to use the
type()of the python value first, before resorting to guessing. The python type is valid, if the Flyte type inferred by the transformer associated with that python type, matches the literal type of the flyte entity.What changes were proposed in this pull request?
Add a better guess.
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 remote entity execution through type engine optimization and strict type checking. The implementation introduces type_match_checking.py and type_engine.py modules, consolidating type operations with improved error handling. Key changes include renaming 'better_guess_type_hint' to 'strict_type_hint_matching', adding detailed error logging, and implementing a fallback mechanism for type guessing. These modifications support various types (enums, unions, collections) while ensuring more precise type validation.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2