Improve podtemplate override logic #3117

arbaobao · 2025-02-07T10:28:19Z

Tracking issue

Issue#5683

Why are the changes needed?

If overrides podTemplate is not defined, tCtx.TaskExecutionMetadata().GetOverrides().GetPodTemplate() should be nil.
Remove duplicate code.

What changes were proposed in this pull request?

If overrides pod_template is none ,return pod_template=None in TaskNodeOverrides.

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

Enhancement to workflow module's pod template override handling and authentication system. Introduces thread-safe factory patterns, environment validation, and secure pip secret mount support for image building. Improves type system handling with simplified pickle detection and type extraction. Includes restructured TaskNodeOverrides class with proper serialization support and comprehensive test coverage.

Unit tests added: True

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

Signed-off-by: Nelson Chen <[email protected]>

flyte-bot · 2025-02-07T10:29:35Z

Code Review Agent Run #833d42

Actionable Suggestions - 1

flytekit/models/core/workflow.py - 1
- Consider extracting pod template override logic · Line 645-659

Review Details

Files reviewed - 1 · Commit Range: f5acd79..190fdfd
- flytekit/models/core/workflow.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: Nelson Chen <[email protected]>

flyte-bot · 2025-02-07T10:32:22Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Pod Template Override Handling	- `workflow.py` - Refactored pod template override logic for better null handling and code organization - `test_workflow.py` - Added test cases to verify pod template override behavior

flytekit/models/core/workflow.py

flyte-bot · 2025-02-07T11:31:37Z

Code Review Agent Run #85621c

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 190fdfd..21c407d
- flytekit/models/core/workflow.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

eapolinario

can you add a few unit tests?

Signed-off-by: Nelson Chen <[email protected]>

codecov · 2025-02-07T20:23:26Z

Codecov Report

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

Project coverage is 79.62%. Comparing base (d732d6f) to head (27d7391).

Files with missing lines	Patch %	Lines
flytekit/models/core/workflow.py	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3117      +/-   ##
==========================================
- Coverage   79.64%   79.62%   -0.03%     
==========================================
  Files         203      203              
  Lines       21621    21625       +4     
  Branches     2788     2789       +1     
==========================================
- Hits        17221    17219       -2     
- Misses       3625     3630       +5     
- Partials      775      776       +1

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

arbaobao · 2025-02-07T20:39:03Z

@eapolinario
I think we already test it in test_task_node_with_overrides, but i didn't handle pod_template in from_flyte_idl method. As a result, pod_template in obj always returns None, regardless of whether it’s None or K8sPod.

After adding pod_template into from_flyte_idl, if pod_template returns a struct rather than None, the test will fail.

Signed-off-by: Nelson Chen <[email protected]>

flyte-bot · 2025-02-07T21:21:51Z

Code Review Agent Run #9c3fa1

Actionable Suggestions - 1

flytekit/models/core/workflow.py - 1
- Consider reducing return statement duplication · Line 669-680

Review Details

Files reviewed - 1 · Commit Range: 21c407d..8b4b063
- flytekit/models/core/workflow.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/models/core/workflow.py

Signed-off-by: Nelson Chen <[email protected]>

…dtemplate_override_logic

Signed-off-by: Nelson Chen <[email protected]>

flyte-bot · 2025-02-11T14:44:33Z

Code Review Agent Run #9b999a

Actionable Suggestions - 0

Additional Suggestions - 9

flytekit/image_spec/image_spec.py - 2
- Consider moving parameters list to constant · Line 158-162
- Consider extracting pip_secret_mounts validation logic · Line 134-141
flytekit/image_spec/default_builder.py - 1
- Consider extracting secret mount string construction · Line 224-229
tests/flytekit/unit/models/core/test_security.py - 1
- Consider adding valid env var tests · Line 57-61
tests/flytekit/unit/models/core/test_workflow.py - 1
- Consider meaningful value instead of empty string · Line 348-348
flytekit/clients/auth_helper.py - 1
- Consider caching authenticator instance creation · Line 126-127
flytekit/core/promise.py - 1
- Consider removing unnecessary type cast · Line 950-950
tests/flytekit/integration/remote/test_remote.py - 1
- Consider more descriptive variable name · Line 581-590
flytekit/clients/grpc_utils/auth_interceptor.py - 1
- Consider consistent private attribute naming · Line 74-74

Review Details

Files reviewed - 21 · Commit Range: 8b4b063..0fb390e
- flytekit/clients/auth_helper.py
- flytekit/clients/grpc_utils/auth_interceptor.py
- flytekit/core/promise.py
- flytekit/core/type_engine.py
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- flytekit/models/security.py
- flytekit/remote/remote.py
- pydoclint-errors-baseline.txt
- tests/flytekit/integration/remote/test_remote.py
- tests/flytekit/unit/clients/auth/test_keyring_store.py
- tests/flytekit/unit/clients/test_auth_helper.py
- tests/flytekit/unit/clients/test_friendly.py
- tests/flytekit/unit/clients/test_raw.py
- tests/flytekit/unit/core/image_spec/test_default_builder.py
- tests/flytekit/unit/core/image_spec/test_image_spec.py
- tests/flytekit/unit/core/test_annotated_bindings.py
- tests/flytekit/unit/core/test_generice_idl_type_engine.py
- tests/flytekit/unit/core/test_type_engine.py
- tests/flytekit/unit/models/core/test_security.py
- tests/flytekit/unit/models/core/test_workflow.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

arbaobao added 2 commits February 7, 2025 02:01

test

f5acd79

Signed-off-by: Nelson Chen <[email protected]>

improve logic

190fdfd

Signed-off-by: Nelson Chen <[email protected]>

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

lint

21c407d

Signed-off-by: Nelson Chen <[email protected]>

flyte-bot reviewed Feb 7, 2025

View reviewed changes

flytekit/models/core/workflow.py Outdated Show resolved Hide resolved

eapolinario reviewed Feb 7, 2025

View reviewed changes

arbaobao added 2 commits February 8, 2025 04:16

Add pod template in TaskNodeOverrides from_flyte_idl

2e84d39

Signed-off-by: Nelson Chen <[email protected]>

lint

0567341

Signed-off-by: Nelson Chen <[email protected]>

lint

8b4b063

Signed-off-by: Nelson Chen <[email protected]>

flyte-bot reviewed Feb 7, 2025

View reviewed changes

flytekit/models/core/workflow.py Show resolved Hide resolved

arbaobao added 3 commits February 11, 2025 21:40

add unit test for podtemplate with_override

1ad5b1c

Signed-off-by: Nelson Chen <[email protected]>

Merge branch 'master' of github.com:flyteorg/flytekit into improve_po…

27d7391

…dtemplate_override_logic

add comment

0fb390e

Signed-off-by: Nelson Chen <[email protected]>

pingsutw approved these changes Feb 11, 2025

View reviewed changes

pingsutw merged commit 13b8e08 into flyteorg:master Feb 11, 2025
109 of 111 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve podtemplate override logic #3117

Improve podtemplate override logic #3117

arbaobao commented Feb 7, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #833d42

flyte-bot commented Feb 7, 2025 •

edited

Loading

Changelist by Bito

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #85621c

eapolinario left a comment

codecov bot commented Feb 7, 2025 •

edited

Loading

arbaobao commented Feb 7, 2025

flyte-bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #9c3fa1

flyte-bot commented Feb 11, 2025 •

edited

Loading

Code Review Agent Run #9b999a

Improve podtemplate override logic #3117

Improve podtemplate override logic #3117

Conversation

arbaobao commented Feb 7, 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 7, 2025 • edited Loading

Code Review Agent Run #833d42

flyte-bot commented Feb 7, 2025 • edited Loading

Changelist by Bito

flyte-bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #85621c

eapolinario left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

arbaobao commented Feb 7, 2025

flyte-bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #9c3fa1

flyte-bot commented Feb 11, 2025 • edited Loading

Code Review Agent Run #9b999a

arbaobao commented Feb 7, 2025 •

edited by flyte-bot

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

codecov bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 7, 2025 •

edited

Loading

flyte-bot commented Feb 11, 2025 •

edited

Loading