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

fix(admin): validate cron expression in launch plan schedule #5951

Conversation

peterxcli
Copy link
Contributor

@peterxcli peterxcli commented Nov 2, 2024

Tracking issue

#4953

Why are the changes needed?

Validate cron expression in launch plan schedule first, preventing "runtime error" when LPs are really scheduled.

What changes were proposed in this pull request?

Validate cron expression in launch plan schedule when they are created or updated.

How was this patch tested?

  1. unit test ✅
  2. manually test ✅

Setup process

  1. Bootstrap cluster
DOCKER_HOST="unix://$(echo ~/.orbstack/run/docker.sock)" flytectl demo start --dev
FLYTECTL_CONFIG=~/.flyte/config-sandbox.yaml
POD_NAMESPACE=flyte flyte start --config flyte-single-binary-local.yaml
  1. Define a invalde cron schedule LP in py
# modify from https://github.com/flyteorg/flytesnacks/blob/0ec8388759d34566a0ffc0c3c2d7443fd4a3a46f/examples/basics/basics/launch_plan.py

from flytekit import CronSchedule, LaunchPlan, current_context

# Import the workflow from the workflow.py file
# in order to create a launch plan for it
from .workflow import simple_wf

# Create a default launch plan with no inputs during serialization
default_lp = LaunchPlan.get_default_launch_plan(current_context(), simple_wf)

# Run the launch plan locally
default_lp(x=[-3, 0, 3], y=[7, 4, -2])

# Create a launch plan and specify the default inputs
simple_wf_lp = LaunchPlan.create(
    name="simple_wf_lp",
    workflow=simple_wf,
    schedule=CronSchedule(schedule="* * * * MON#1"),
    default_inputs={"x": [-3, 0, 3], "y": [7, 4, -2]},
)

# Trigger the launch plan locally
simple_wf_lp()

# Override the defaults as follows
simple_wf_lp(x=[3, 5, 3], y=[-3, 2, -2])

# It's possible to lock launch plan inputs, preventing them from being overridden during execution
simple_wf_lp_fixed_inputs = LaunchPlan.get_or_create(
    name="fixed_inputs", workflow=simple_wf, fixed_inputs={"x": [-3, 0, 3]}
)
  1. Register it!
pyflyte register launch_plan.py

Screenshots

image

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

Copy link

welcome bot commented Nov 2, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

- Add cron expression validation using robfig/cron parser
- Fix test cases to use correct 5-field cron format
- Update invalid cron expression tests to expect errors
- Remove extra asterisk from cron patterns in tests

Signed-off-by: peterxcli <[email protected]>
@peterxcli peterxcli force-pushed the 4953-flytescheduler-cron-expression-validation-process branch from 4b340ca to 5a6cdbe Compare November 2, 2024 04:19
Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.87%. Comparing base (25cfe16) to head (e467337).
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5951       +/-   ##
===========================================
+ Coverage   33.09%   50.87%   +17.77%     
===========================================
  Files        1013     1164      +151     
  Lines      107489    89630    -17859     
===========================================
+ Hits        35570    45596    +10026     
+ Misses      68767    40005    -28762     
- Partials     3152     4029      +877     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.07% <100.00%> (?)
unittests-flytecopilot 22.23% <ø> (ø)
unittests-flytectl 62.39% <ø> (-0.05%) ⬇️
unittests-flyteidl ?
unittests-flyteplugins 53.84% <ø> (ø)
unittests-flytepropeller 42.90% <ø> (ø)
unittests-flytestdlib 55.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@kumare3
Copy link
Contributor

kumare3 commented Nov 2, 2024

Lgtm

Reorder imports to follow gci standards with groups:
1. stdlib
2. third-party
3. internal/flyteorg packages

Signed-off-by: peterxcli <[email protected]>
@peterxcli peterxcli force-pushed the 4953-flytescheduler-cron-expression-validation-process branch from 64e24f4 to 3957a55 Compare November 3, 2024 04:31
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, could you update the PR description

@peterxcli
Copy link
Contributor Author

@pingsutw Thanks for reviewing this

PR desc updated!

please take a look, thanks!

@peterxcli
Copy link
Contributor Author

peterxcli commented Nov 8, 2024

Is the codecov decrement a normal situation? Maybe this additional validation make some test fail earlier, which are also expected to fail? Or just a unrelated flake?

@peterxcli
Copy link
Contributor Author

Oh ok, checks passed after merging master branch😅

@Future-Outlier Future-Outlier merged commit fa01e76 into flyteorg:master Nov 11, 2024
51 checks passed
Copy link

welcome bot commented Nov 11, 2024

Congrats on merging your first pull request! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants