Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make error messages in LaunchPlan.get_or_create actionable #2451

Merged
merged 1 commit into from
May 30, 2024

Conversation

fg91
Copy link
Member

@fg91 fg91 commented May 30, 2024

Why are the changes needed?

When registering their applications (tasks, workflows, launchplans), our users are regularly confused by the following exception:

~ pyflyte register ...
  ...
  File ".../wf.py", line 22, in <module>
    lp = LaunchPlan.get_or_create(

  File ".../flytekit/core/launch_plan.py", line 284, in get_or_create
    raise AssertionError("The cached values aren't the same as the current call arguments")

Often, our users think this was related to Flyte's caching mechanism, steering their debugging in the wrong direction.

Actually, the problem occurs when ...

  1. ... trying to use the same launch plan name for two different workflows

    @workflow
    def wf():
    
    @workflow
    def wf2():
    
    lp = LaunchPlan.get_or_create(
        wf,
        "my_wf",
    )
    
    lp = LaunchPlan.get_or_create(
        wf2,
        "my_wf",
    )
  2. ... or trying to create two launchplans with the same name and for the same workflow but with different arguments:

    lp = LaunchPlan.get_or_create(
        wf,
        "my_wf",
    )
    
    lp = LaunchPlan.get_or_create(
        wf,
        "my_wf",
        schedule=CronSchedule(schedule="0 0 * * *"),
    )

In these condensed examples the error is obvious, in a larger code base with launch plans defined in different modules it often is not.

What changes were proposed in this pull request?

This PR makes makes the error message more actionable. In the two examples above:

Trying to create two launch plans both named 'my_wf' for the workflows 'test.wf.wf2' and 'test.wf.wf' - please ensure unique names.
Trying to create two launch plans for workflow 'test.wf.wf' both named 'my_wf' but with different values for 'schedule' - please use different launch plan names.

How was this patch tested?

Tested using the examples above.

Check all the applicable boxes

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

@fg91 fg91 force-pushed the fg91/ref/lp-get-create-actionable-error-msg branch from bdbe26a to 8fb6d47 Compare May 30, 2024 17:53
Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 75.92%. Comparing base (1a01368) to head (8fb6d47).
Report is 2 commits behind head on master.

Files Patch % Lines
flytekit/core/launch_plan.py 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2451      +/-   ##
==========================================
+ Coverage   72.22%   75.92%   +3.69%     
==========================================
  Files         182      182              
  Lines       18475    18473       -2     
  Branches     3608     3609       +1     
==========================================
+ Hits        13344    14025     +681     
+ Misses       4499     3837     -662     
+ Partials      632      611      -21     

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

@fg91 fg91 added the Improve error handling Improve error message label May 30, 2024
@fg91 fg91 self-assigned this May 30, 2024
@fg91 fg91 merged commit 59413c2 into master May 30, 2024
46 of 48 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improve error handling Improve error message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants