Prevent register_launch_plan from re-registering already registered workflow #3049

wild-endeavor · 2025-01-11T01:04:45Z

Tracking issue

Linking this to flyteorg/flyte#6062 which is related.

Why are the changes needed?

Please see comment in the linked issue, copied here:

Assume in the past my_wf has already been registered with version ksD2LBYDu5PrNtNnnpd95Q

Now if you try to do:

from wfs import my_wf

lp = LaunchPlan.get_or_create(workflow=my_wf, name="other_lp_for_hello2", default_inputs={})
remote = FlyteRemote(...)
remote.register_launch_plan(entity=lp, version="ksD2LBYDu5PrNtNnnpd95Q")
...

The issue is that register_launch_plan will currently try to register the underlying workflow again. This would be fine, except that the serialization settings that are used may be completely different. Even if it's exactly the same, this is a waste of time because the best case scenario is that Admin returns already exists for the workflow and every underlying task (and subwf, etc.).

We should check first to see if the workflow already exists with that version, and only register the launch plan if the workflow already exists.

Skipping re-registration of the workflow does mean that users who were relying on this re-registration (and were somehow reconstructing serialization settings to be identical) to detect changes in workflow/task structure will no longer see errors, but that seems too far-fetched.

What changes were proposed in this pull request?

Check if the underlying workflow exists, and if so only register the launch plan.
Also pass through an argument (create_default_launchplan) in _serialize_and_register which for some reason wasn't used. Just a nit, default was always just True.

How was this patch tested?

Tested with local sandbox.

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 streamlines the launch plan registration process by implementing checks to prevent redundant workflow registrations. The changes enhance FlyteRemote by adding workflow existence verification and optimizing the registration flow. The implementation includes proper handling of create_default_launchplan parameter, improved error handling with specific exceptions, and better error messaging.

Unit tests added: True

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

…w if it already exists Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-01-11T01:04:57Z

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-01-11T01:07:48Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 77.75%. Comparing base (dfa8f04) to head (309c5a7).

Files with missing lines	Patch %	Lines
flytekit/remote/remote.py	53.84%	4 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (dfa8f04) and HEAD (309c5a7). Click for more details.

HEAD has 49 uploads less than BASE

Flag BASE (dfa8f04) HEAD (309c5a7)

53 4

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3049      +/-   ##
==========================================
- Coverage   83.47%   77.75%   -5.72%     
==========================================
  Files         319      226      -93     
  Lines       26427    22959    -3468     
  Branches     2744     2746       +2     
==========================================
- Hits        22060    17852    -4208     
- Misses       3591     4346     +755     
+ Partials      776      761      -15

☔ 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-01-11T02:46:37Z

Code Review Agent Run #345323

Actionable Suggestions - 1

flytekit/remote/remote.py - 1
- Consider extracting workflow registration logic · Line 1290-1305

Additional Suggestions - 1

flytekit/remote/remote.py - 1
- Consider more descriptive parameter name · Line 1282-1283

Review Details

Files reviewed - 2 · Commit Range: 2641b3e..84b90fc
- flytekit/remote/remote.py
- tests/flytekit/unit/remote/test_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-11T02:49:04Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Optimized Launch Plan Registration	- `remote.py` - Added workflow existence check and improved launch plan registration logic - `test_remote.py` - Added unit tests for optimized launch plan registration

flytekit/remote/remote.py

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

pingsutw · 2025-01-13T18:14:41Z

flytekit/remote/remote.py

+                launch_plan_model, serialization_settings, version, create_default_launchplan=False
+            )
+            if ident is None:
+                raise ValueError("Failed to register launch plan")


Should we raise this error in self.raw_register? Also, are we able to add the error message here? Something like Failed to register launch plan with err: {err}

i say we keep this here until we can make the raw_register signature not an optional.

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

…orkflow (#3049) Signed-off-by: Yee Hing Tong <[email protected]>

…orkflow (#3049) (#3054) Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot · 2025-01-13T19:15:08Z

Code Review Agent Run #8550a9

Actionable Suggestions - 2

flytekit/remote/remote.py - 2
- Consider broader exception handling for API call · Line 1270-1271
- Consider more specific exception type · Line 1319-1319

Review Details

Files reviewed - 1 · Commit Range: 84b90fc..b78e707
- 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-13T19:17:51Z

flytekit/remote/remote.py

+        except FlyteEntityNotExistException:
+            return False


Consider broader exception handling for API call

Consider adding error handling for other potential exceptions besides FlyteEntityNotExistException when calling get_workflow. Network errors or other API issues could occur.

Code suggestion

Check the AI-generated fix before applying

Suggested change

except FlyteEntityNotExistException:

return False

except FlyteEntityNotExistException:

return False

except Exception as e:

logger.error(f"Error checking workflow existence: {e}")

return False

Code Review Run #8550a9

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

it was incorrectly flagged

flyte-bot · 2025-01-13T19:17:52Z

flytekit/remote/remote.py

+                launch_plan_model, serialization_settings, version, create_default_launchplan=False
+            )
+            if ident is None:
+                raise ValueError("Failed to register launch plan, identifier returned was empty...")


Consider more specific exception type

Consider using a more specific exception type like FlyteRegistrationException instead of ValueError when handling registration failures. This would make error handling more targeted.

Code suggestion

Check the AI-generated fix before applying

Suggested change

raise ValueError("Failed to register launch plan, identifier returned was empty...")

raise FlyteRegistrationException("Failed to register launch plan, identifier returned was empty...")

Code Review Run #8550a9

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

it was incorrectly flagged

…orkflow (flyteorg#3049) Signed-off-by: Yee Hing Tong <[email protected]>

register_launch_plan shouldn't try to register the underlying workflo…

2641b3e

…w if it already exists Signed-off-by: Yee Hing Tong <[email protected]>

mocked unit test

84b90fc

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

wild-endeavor changed the title ~~register_launch_plan shouldn't try to register the underlying workflo…~~ Prevent register_launch_plan from re-registering already registered workflow Jan 11, 2025

wild-endeavor marked this pull request as ready for review January 11, 2025 01:46

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

flyte-bot reviewed Jan 11, 2025

View reviewed changes

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

refactor and comments

309c5a7

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

pingsutw previously approved these changes Jan 13, 2025

View reviewed changes

add more msg into error

b78e707

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

wild-endeavor dismissed pingsutw’s stale review via b78e707 January 13, 2025 18:27

pingsutw approved these changes Jan 13, 2025

View reviewed changes

wild-endeavor merged commit 84eff8e into master Jan 13, 2025
102 of 104 checks passed

wild-endeavor added a commit that referenced this pull request Jan 13, 2025

Prevent register_launch_plan from re-registering already registered w…

f9f382d

…orkflow (#3049) Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor mentioned this pull request Jan 13, 2025

Prevent register_launch_plan from re-registering already registered workflows #3054

Merged

wild-endeavor added a commit that referenced this pull request Jan 13, 2025

Prevent register_launch_plan from re-registering already registered w…

ba9892d

…orkflow (#3049) (#3054) Signed-off-by: Yee Hing Tong <[email protected]>

flyte-bot reviewed Jan 13, 2025

View reviewed changes

shuyingliang pushed a commit to shuyingliang/flytekit that referenced this pull request Jan 22, 2025

Prevent register_launch_plan from re-registering already registered w…

34ef354

…orkflow (flyteorg#3049) Signed-off-by: Yee Hing Tong <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent register_launch_plan from re-registering already registered workflow #3049

Prevent register_launch_plan from re-registering already registered workflow #3049

wild-endeavor commented Jan 11, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 11, 2025

codecov bot commented Jan 11, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading

Code Review Agent Run #345323

flyte-bot commented Jan 11, 2025 •

edited

Loading

Changelist by Bito

pingsutw Jan 13, 2025

wild-endeavor Jan 13, 2025

flyte-bot commented Jan 13, 2025 •

edited

Loading

Code Review Agent Run #8550a9

flyte-bot Jan 13, 2025

flyte-bot Jan 13, 2025

	raise ValueError("Failed to register launch plan, identifier returned was empty...")
	raise FlyteRegistrationException("Failed to register launch plan, identifier returned was empty...")

Prevent register_launch_plan from re-registering already registered workflow #3049

Prevent register_launch_plan from re-registering already registered workflow #3049

Conversation

wild-endeavor commented Jan 11, 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 11, 2025

Code Review Agent Run Status

codecov bot commented Jan 11, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 11, 2025 • edited Loading

Code Review Agent Run #345323

flyte-bot commented Jan 11, 2025 • edited Loading

Changelist by Bito

pingsutw Jan 13, 2025

Choose a reason for hiding this comment

wild-endeavor Jan 13, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 13, 2025 • edited Loading

Code Review Agent Run #8550a9

flyte-bot Jan 13, 2025

Choose a reason for hiding this comment

flyte-bot Jan 13, 2025

Choose a reason for hiding this comment

wild-endeavor commented Jan 11, 2025 •

edited by flyte-bot

Loading

codecov bot commented Jan 11, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading

flyte-bot commented Jan 13, 2025 •

edited

Loading