Improve Union Transformer Ambiguous Error Message #3076

Future-Outlier · 2025-01-22T04:02:47Z

Tracking issue

https://flyte-org.slack.com/archives/CP2HDHKE1/p1737474492222339

Why are the changes needed?

Users can debug more effectively if having potential types.

What changes were proposed in this pull request?

add potential types variables when doing type transform
print potential types if there's an ambiguous case.

How was this patch tested?

unit test and local execution.

from flytekit import task
from dataclasses import dataclass
from typing import Union

@dataclass
class A:
    a: int

@dataclass
class B:
    a: int

@task
def t1() -> Union[A, B]:
    return A(3)

if __name__ == "__main__":
    print(t1())

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

Summary by Bito

Enhanced error messaging system in Flytekit for ambiguous union type transformations, providing more detailed type information and context. The update includes improved TypeError messages in type_engine.py with comprehensive lists of potential matching types and structurally similar types. These changes aim to improve developer debugging experience.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 1

Signed-off-by: Future-Outlier <eric901201@gmail.com>

flyte-bot · 2025-01-22T04:03:00Z

Code Review Agent Run #da8f08

Actionable Suggestions - 1

tests/flytekit/unit/core/test_unions.py - 1
- Consider more specific error message assertion · Line 160-161

Review Details

Files reviewed - 2 · Commit Range: 8052d77..8052d77
- flytekit/core/type_engine.py
- tests/flytekit/unit/core/test_unions.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-01-22T04:06:20Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Union Type Error Messaging	- `type_engine.py` - Added potential types tracking and improved error message for ambiguous union types - `test_unions.py` - Added test cases to verify enhanced error messages for ambiguous union types

flyte-bot · 2025-01-22T04:06:21Z

tests/flytekit/unit/core/test_unions.py

+        match=("Potential types:")
+    ):


Consider more specific error message assertion

The error message assertion match=("Potential types:") seems too generic. Consider making it more specific to match the actual error message that would be raised when there are ambiguous union types.

Code suggestion

Check the AI-generated fix before applying

Suggested change

match=("Potential types:")

):

match="Cannot determine which type to choose from Union[A, B] for value of type A. Potential types:"

):

Code Review Run #da8f08

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

codecov · 2025-01-22T04:07:50Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 51.96%. Comparing base (a86048a) to head (3fe9801).
Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
flytekit/core/type_engine.py	0.00%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (a86048a) and HEAD (3fe9801). Click for more details.

HEAD has 18 uploads less than BASE

Flag BASE (a86048a) HEAD (3fe9801)

20 2

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3076       +/-   ##
===========================================
- Coverage   77.44%   51.96%   -25.49%     
===========================================
  Files         238      202       -36     
  Lines       23158    21474     -1684     
  Branches     2760     2768        +8     
===========================================
- Hits        17935    11158     -6777     
- Misses       4412     9696     +5284     
+ Partials      811      620      -191

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Future-Outlier <eric901201@gmail.com>

flyte-bot · 2025-01-22T05:18:35Z

Code Review Agent Run #5172b2

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 8052d77..b9298cd
- tests/flytekit/unit/core/test_unions.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

thomasjpfan

For your example:

@dataclass
class A:
    a: int

@dataclass
class B:
    a: int

what should we recommend the user to make it work?

thomasjpfan · 2025-01-24T22:10:07Z

flytekit/core/type_engine.py

            except Exception as e:
                logger.warning(
                    f"UnionTransformer failed attempt to convert from {python_val} to {t} error: {e}",
                )
                continue

        if is_ambiguous:
-            raise TypeError("Ambiguous choice of variant for union type")
+            raise TypeError(f"Ambiguous choice of variant for union type.\n" f"Potential types: {potential_types}\n")


Can we add more to this error message stating why they are ambiguous? Something like:

These types are structurally the same, because it's attributes have the same names and associated types.

Otherwise, it's hard to tell from the error what is wrong.

love u thomas, it's upadted

Signed-off-by: Future-Outlier <eric901201@gmail.com>

flyte-bot · 2025-02-04T00:52:56Z

Code Review Agent Run #cc92f6

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: b9298cd..3fe9801
- flytekit/core/type_engine.py
- tests/flytekit/unit/core/test_unions.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

* Improve Union Transformer Ambiguous Error Message Signed-off-by: Future-Outlier <eric901201@gmail.com> * better error msg assertion Signed-off-by: Future-Outlier <eric901201@gmail.com> --------- Signed-off-by: Future-Outlier <eric901201@gmail.com> Signed-off-by: Umer Ahmad <umer2ahmad@gmail.com>

Improve Union Transformer Ambiguous Error Message

8052d77

Signed-off-by: Future-Outlier <eric901201@gmail.com>

Future-Outlier requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla and thomasjpfan as code owners January 22, 2025 04:02

flyte-bot reviewed Jan 22, 2025

View reviewed changes

better error msg assertion

b9298cd

Signed-off-by: Future-Outlier <eric901201@gmail.com>

Future-Outlier enabled auto-merge (squash) January 24, 2025 06:41

thomasjpfan reviewed Jan 24, 2025

View reviewed changes

update thomas's advice

3fe9801

Signed-off-by: Future-Outlier <eric901201@gmail.com>

pingsutw approved these changes Feb 4, 2025

View reviewed changes

Future-Outlier merged commit 6bdf411 into master Feb 4, 2025
107 of 112 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Union Transformer Ambiguous Error Message #3076

Improve Union Transformer Ambiguous Error Message #3076

Future-Outlier commented Jan 22, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 22, 2025 •

edited

Loading

Code Review Agent Run #da8f08

flyte-bot commented Jan 22, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

flyte-bot commented Jan 22, 2025 •

edited

Loading

Code Review Agent Run #5172b2

thomasjpfan left a comment

thomasjpfan Jan 24, 2025

Future-Outlier Feb 3, 2025

flyte-bot commented Feb 4, 2025 •

edited

Loading

Code Review Agent Run #cc92f6

Improve Union Transformer Ambiguous Error Message #3076

Improve Union Transformer Ambiguous Error Message #3076

Conversation

Future-Outlier commented Jan 22, 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

Summary by Bito

flyte-bot commented Jan 22, 2025 • edited Loading

Code Review Agent Run #da8f08

flyte-bot commented Jan 22, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 22, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 22, 2025 • edited Loading

Code Review Agent Run #5172b2

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan Jan 24, 2025

Choose a reason for hiding this comment

Future-Outlier Feb 3, 2025

Choose a reason for hiding this comment

flyte-bot commented Feb 4, 2025 • edited Loading

Code Review Agent Run #cc92f6

Future-Outlier commented Jan 22, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 22, 2025 •

edited

Loading

flyte-bot commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

flyte-bot commented Jan 22, 2025 •

edited

Loading

flyte-bot commented Feb 4, 2025 •

edited

Loading