Attempt to use more specific transformer when executing remote entities #3111

wild-endeavor · 2025-02-05T23:56:45Z

Tracking issue

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

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

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

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-05T23:56:58Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

codecov · 2025-02-06T00:17:19Z

Codecov Report

Attention: Patch coverage is 74.35897% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.11%. Comparing base (22acc2c) to head (7e8fd7c).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
flytekit/core/type_match_checking.py	73.33%	5 Missing and 3 partials ⚠️
flytekit/core/type_engine.py	71.42%	1 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-07T03:56:53Z

Code Review Agent Run #e0c471

Actionable Suggestions - 1

flytekit/remote/remote.py - 1
- Consider adding error handling for transformer · Line 255-265

Review Details

Files reviewed - 3 · Commit Range: ad7e95a..400a996
- flytekit/core/type_match_checking.py
- flytekit/remote/remote.py
- tests/flytekit/unit/core/test_type_match_checking.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-02-07T04:01:10Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Type Matching System	- `type_engine.py` - Added strict type hint matching functionality for better type inference - `type_match_checking.py` - New module for literal type matching with support for collections, enums, and unions - `remote.py` - Updated remote execution to use strict type matching for better type resolution
Testing - Type Matching Test Coverage	- `test_type_engine.py` - Added tests for strict type matching functionality - `test_type_match_checking.py` - New test suite for literal type matching across different data types

flytekit/remote/remote.py

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-07T20:11:52Z

Code Review Agent Run #2f3b74

Actionable Suggestions - 2

tests/flytekit/unit/core/test_type_engine.py - 1
- Consider safer dictionary key removal approach · Line 3813-3813
flytekit/core/type_engine.py - 1
- Consider adding error handling for transformers · Line 2433-2440

Review Details

Files reviewed - 4 · Commit Range: 400a996..7e8fd7c
- flytekit/core/type_engine.py
- flytekit/remote/remote.py
- tests/flytekit/unit/core/test_type_engine.py
- tests/flytekit/unit/core/test_type_match_checking.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

tests/flytekit/unit/core/test_type_engine.py

flytekit/core/type_engine.py

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-07T22:11:51Z

Code Review Agent Run #e0383a

Actionable Suggestions - 1

tests/flytekit/unit/core/test_type_engine.py - 1
- Consider adding edge cases to test · Line 3817-3817

Review Details

Files reviewed - 2 · Commit Range: 7e8fd7c..2e78438
- flytekit/core/type_engine.py
- tests/flytekit/unit/core/test_type_engine.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

tests/flytekit/unit/core/test_type_engine.py

eapolinario

one tiny comment, otherwise LGTM. Thank you!

flytekit/core/type_engine.py

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-10T19:22:25Z

Code Review Agent Run #b17d18

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 2e78438..57c0e7e
- flytekit/core/type_engine.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-02-11T06:10:56Z

Code Review Agent Run #67094d

Actionable Suggestions - 2

flytekit/remote/remote.py - 1
- Consider more specific error handling · Line 1517-1522
flytekit/core/type_engine.py - 1
- Consider impact of strict type matching · Line 2427-2427

Additional Suggestions - 1

tests/flytekit/unit/core/test_type_engine.py - 1
- Consider renaming test for consistency · Line 3786-3786

Review Details

Files reviewed - 3 · Commit Range: 57c0e7e..db2c3ab
- flytekit/core/type_engine.py
- flytekit/remote/remote.py
- tests/flytekit/unit/core/test_type_engine.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flytekit/remote/remote.py

flytekit/core/type_engine.py

fmt

ad7e95a

Signed-off-by: Yee Hing Tong <[email protected]>

add testing

400a996

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor marked this pull request as ready for review February 7, 2025 02:54

wild-endeavor requested review from kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners February 7, 2025 02:54

flyte-bot reviewed Feb 7, 2025

View reviewed changes

flytekit/remote/remote.py Outdated Show resolved Hide resolved

wild-endeavor added 3 commits February 7, 2025 11:07

add test & comment

9b5f036

Signed-off-by: Yee Hing Tong <[email protected]>

lint

8468da4

Signed-off-by: Yee Hing Tong <[email protected]>

import

7e8fd7c

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot reviewed Feb 7, 2025

View reviewed changes

tests/flytekit/unit/core/test_type_engine.py Show resolved Hide resolved

flyte-bot reviewed Feb 7, 2025

View reviewed changes

flytekit/core/type_engine.py Outdated Show resolved Hide resolved

fix

2e78438

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot reviewed Feb 7, 2025

View reviewed changes

tests/flytekit/unit/core/test_type_engine.py Show resolved Hide resolved

eapolinario reviewed Feb 8, 2025

View reviewed changes

flytekit/core/type_engine.py Outdated Show resolved Hide resolved

Future-Outlier mentioned this pull request Feb 10, 2025

[BUG] [flytekit] cannot pass dataclass with Optional[list] to FlyteRemote.execute flyteorg/flyte#6057

Open

2 tasks

pr comment

57c0e7e

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor enabled auto-merge (squash) February 10, 2025 18:15

pr comment

db2c3ab

Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot reviewed Feb 11, 2025

View reviewed changes

flytekit/remote/remote.py Show resolved Hide resolved

flyte-bot reviewed Feb 11, 2025

View reviewed changes

flytekit/core/type_engine.py Show resolved Hide resolved

eapolinario approved these changes Feb 11, 2025

View reviewed changes

wild-endeavor merged commit 7f58efe into master Feb 11, 2025
109 of 111 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to use more specific transformer when executing remote entities #3111

Attempt to use more specific transformer when executing remote entities #3111

wild-endeavor commented Feb 5, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Feb 5, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #e0c471

flyte-bot commented Feb 7, 2025 •

edited

Loading

Changelist by Bito

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #2f3b74

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #e0383a

eapolinario left a comment

flyte-bot commented Feb 10, 2025 •

edited

Loading

Code Review Agent Run #b17d18

flyte-bot commented Feb 11, 2025 •

edited

Loading

Code Review Agent Run #67094d

Attempt to use more specific transformer when executing remote entities #3111

Attempt to use more specific transformer when executing remote entities #3111

Conversation

wild-endeavor commented Feb 5, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

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

flyte-bot commented Feb 5, 2025

Code Review Agent Run Status

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

flyte-bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #e0c471

flyte-bot commented Feb 7, 2025 • edited Loading

Changelist by Bito

flyte-bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #2f3b74

flyte-bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #e0383a

eapolinario left a comment

Choose a reason for hiding this comment

flyte-bot commented Feb 10, 2025 • edited Loading

Code Review Agent Run #b17d18

flyte-bot commented Feb 11, 2025 • edited Loading

Code Review Agent Run #67094d

wild-endeavor commented Feb 5, 2025 •

edited by flyte-bot

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 10, 2025 •

edited

Loading

flyte-bot commented Feb 11, 2025 •

edited

Loading