[WIP][Flytekit] Add variable_map in LiteralResolver for FlyteRemote.get Error on Dataclass/Pydantic Models #3031

mao3267 · 2025-01-04T03:19:52Z

Tracking issue

Why are the changes needed?

While the task input/output is represented as a dataclass or Pydantic model, using the get function to fetch the FlyteRemote execution output will fail due to the absence of the variable_map information. To address this issue, we aim to provide the input/output variable_map in LiteralResolver to address this issue.

What changes were proposed in this pull request?

Add variable_map in LiteralResolver
Unit Tests should be added

How was this patch tested?

TODO

Setup process

git clone https://github.com/flyteorg/flytekit.git
gh pr checkout 2933
pip install -e .

Screenshots

TODO

Check all the applicable boxes

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

Related PRs

flyteorg/flyte#6136

Docs link

Summary by Bito

Enhanced FlyteRemote's LiteralResolver to improve handling of dataclass and Pydantic model inputs/outputs. Implementation includes proper handling of variable map information from data response, with null checks for variable_map field to prevent access issues. The changes standardize the use of LiteralsResolver and remove redundant empty interface checks, resulting in more robust handling of complex data types.

Unit tests added: False

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

Signed-off-by: mao3267 <[email protected]>

…-dataproxy-variable-map

flyte-bot · 2025-01-04T03:20:05Z

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].

…-dataproxy-variable-map

flyte-bot · 2025-01-16T04:56:07Z

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].

flyte-bot · 2025-01-17T02:15:34Z

Code Review Agent Run #6ace01

Actionable Suggestions - 1

flytekit/remote/remote.py - 1
- Consider adding null check for variable_map · Line 364-365

Review Details

Files reviewed - 1 · Commit Range: a88827c..3a61ac9
- flytekit/remote/remote.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-17T02:17:13Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - Fix Dataclass/Pydantic Model Output Retrieval	- `remote.py` - Added variable_map handling in LiteralResolver for proper dataclass/Pydantic model output processing

flyte-bot · 2025-01-17T02:17:15Z

flytekit/remote/remote.py

+                vm = VariableMap.from_flyte_idl(data_response.variable_map)
+                return LiteralsResolver(lm.literals, vm.variables)


Consider adding null check for variable_map

Consider adding null check for data_response.variable_map before accessing it. The variable_map field might be optional and accessing it without validation could cause issues.

Code suggestion

Check the AI-generated fix before applying

Suggested change

vm = VariableMap.from_flyte_idl(data_response.variable_map)

return LiteralsResolver(lm.literals, vm.variables)

variables = {}

if data_response.HasField("variable_map"):

vm = VariableMap.from_flyte_idl(data_response.variable_map)

variables = vm.variables

return LiteralsResolver(lm.literals, variables)

Code Review Run #6ace01

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

it was incorrectly flagged

Signed-off-by: mao3267 <[email protected]>

flyte-bot · 2025-01-17T03:37:37Z

Code Review Agent Run #7c00db

Actionable Suggestions - 1

flytekit/remote/remote.py - 1
- Consider consolidating input/output handling logic · Line 2527-2539

Review Details

Files reviewed - 1 · Commit Range: 3a61ac9..6072099
- flytekit/remote/remote.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-17T03:40:43Z

flytekit/remote/remote.py

+        # 如果 interface.inputs 为空,则只返回 literals
+        if not interface.inputs:
+            execution._inputs = input_literal_map.literals
+        else:
+            execution._inputs = LiteralsResolver(input_literal_map.literals, interface.inputs, self.context)

        if execution.is_done and not execution.error:
            output_literal_map = self._get_output_literal_map(execution_data)
-            execution._outputs = LiteralsResolver(output_literal_map.literals, interface.outputs, self.context)
+            # 如果 interface.outputs 为空,则只返回 literals
+            if not interface.outputs:
+                execution._outputs = output_literal_map.literals
+            else:
+                execution._outputs = LiteralsResolver(output_literal_map.literals, interface.outputs, self.context)


Consider consolidating input/output handling logic

Consider consolidating the input/output handling logic to avoid code duplication. The same pattern is repeated for both inputs and outputs.

Code suggestion

Check the AI-generated fix before applying

- # 如果 interface.inputs 为空,则只返回 literals - if not interface.inputs: - execution._inputs = input_literal_map.literals - else: - execution._inputs = LiteralsResolver(input_literal_map.literals, interface.inputs, self.context) - - if execution.is_done and not execution.error: - output_literal_map = self._get_output_literal_map(execution_data) - # 如果 interface.outputs 为空,则只返回 literals - if not interface.outputs: - execution._outputs = output_literal_map.literals - else: - execution._outputs = LiteralsResolver(output_literal_map.literals, interface.outputs, self.context) + def _assign_literals(literal_map, interface_vars, target_attr): + if not interface_vars: + setattr(execution, target_attr, literal_map.literals) + else: + setattr(execution, target_attr, LiteralsResolver(literal_map.literals, interface_vars, self.context)) + + _assign_literals(input_literal_map, interface.inputs, '_inputs') + + if execution.is_done and not execution.error: + output_literal_map = self._get_output_literal_map(execution_data) + _assign_literals(output_literal_map, interface.outputs, '_outputs')

Code Review Run #7c00db

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

it was incorrectly flagged

Signed-off-by: mao3267 <[email protected]>

flyte-bot · 2025-01-17T04:55:34Z

Code Review Agent Run #d1b3c9

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 6072099..270105d
- flytekit/remote/remote.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

mao3267 added 3 commits December 24, 2024 20:58

feat: add variable_map in data response

a88827c

Signed-off-by: mao3267 <[email protected]>

fix: make lint

e0b8f3a

Signed-off-by: mao3267 <[email protected]>

Merge branch 'master' of https://github.com/mao3267/flytekit into add…

4c73d6b

…-dataproxy-variable-map

mao3267 mentioned this pull request Jan 4, 2025

[WIP][Flyteadmin] Add variablemap in dataproxy for dataclass/pydantic flyteorg/flyte#6136

Draft

3 tasks

Future-Outlier self-assigned this Jan 13, 2025

Merge branch 'master' of https://github.com/mao3267/flytekit into add…

3a61ac9

…-dataproxy-variable-map

mao3267 marked this pull request as ready for review January 17, 2025 01:10

mao3267 requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners January 17, 2025 01:10

flyte-bot reviewed Jan 17, 2025

View reviewed changes

fix: add null check for variable map

6072099

Signed-off-by: mao3267 <[email protected]>

flyte-bot reviewed Jan 17, 2025

View reviewed changes

fix: changed incorrectly

270105d

Signed-off-by: mao3267 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][Flytekit] Add variable_map in LiteralResolver for FlyteRemote.get Error on Dataclass/Pydantic Models #3031

[WIP][Flytekit] Add variable_map in LiteralResolver for FlyteRemote.get Error on Dataclass/Pydantic Models #3031

mao3267 commented Jan 4, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 4, 2025

flyte-bot commented Jan 16, 2025

flyte-bot commented Jan 17, 2025 •

edited

Loading

Code Review Agent Run #6ace01

flyte-bot commented Jan 17, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 17, 2025

flyte-bot commented Jan 17, 2025 •

edited

Loading

Code Review Agent Run #7c00db

flyte-bot Jan 17, 2025

flyte-bot commented Jan 17, 2025 •

edited

Loading

Code Review Agent Run #d1b3c9

		vm = VariableMap.from_flyte_idl(data_response.variable_map)
		return LiteralsResolver(lm.literals, vm.variables)

-                vm = VariableMap.from_flyte_idl(data_response.variable_map)
-                return LiteralsResolver(lm.literals, vm.variables)
+                variables = {}
+                if data_response.HasField("variable_map"):
+                    vm = VariableMap.from_flyte_idl(data_response.variable_map)
+                    variables = vm.variables
+                return LiteralsResolver(lm.literals, variables)

[WIP][Flytekit] Add variable_map in LiteralResolver for FlyteRemote.get Error on Dataclass/Pydantic Models #3031

Are you sure you want to change the base?

[WIP][Flytekit] Add variable_map in LiteralResolver for FlyteRemote.get Error on Dataclass/Pydantic Models #3031

Conversation

mao3267 commented Jan 4, 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 Jan 4, 2025

Code Review Agent Run Status

flyte-bot commented Jan 16, 2025

Code Review Agent Run Status

flyte-bot commented Jan 17, 2025 • edited Loading

Code Review Agent Run #6ace01

flyte-bot commented Jan 17, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 17, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 17, 2025 • edited Loading

Code Review Agent Run #7c00db

flyte-bot Jan 17, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 17, 2025 • edited Loading

Code Review Agent Run #d1b3c9

mao3267 commented Jan 4, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading

flyte-bot commented Jan 17, 2025 •

edited

Loading