Support Styx style schedule #83

honnix · 2020-09-29T09:59:35Z

TL;DR

When integrating with our scheduler Styx, we will need to extend the semantics of schedule to support Styx cron syntax, cron aliases, as well as offset.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Complete description and details can be found in the issue description.

Tracking Issue

flyteorg/flyte#529

Follow-up issue

katrogan · 2020-09-29T16:30:13Z

hey @honnix mind bumping the version here and here as part of your change?

honnix · 2020-09-29T16:41:08Z

Sure I will fix that.

protos/flyteidl/admin/schedule.proto

For example: https://github.com/spotify/styx#schedule-string

honnix · 2020-09-29T18:12:36Z

I made the changes as suggested. I will work on flyteadmin later to reject launch plan with this kind of schedule, then probably there is no need to change flyteconsole to display the schedule. Another PR will be made in flytekit to propagate the proto.

honnix · 2020-09-29T18:30:45Z

I forgot to make generate again. :( Sorry and will fix.

honnix · 2020-09-30T04:47:57Z

Does this look more reasonable now?

kumare3 · 2020-09-30T18:32:56Z

protos/flyteidl/admin/schedule.proto

@@ -16,6 +16,12 @@ message FixedRate {
    FixedRateUnit unit = 2;
 }

+// Supported only by some schedulers for example Spotify/Styx. Your platform may not have this enabled.
+message CronScheduleWithOffset {
+    string schedule = 1;


is the schedule here actually the expression?
Also, this is pretty good, i think eventually we should just deprecate cron_expression for CronScheduleWithOffset (just offset will be 0 by default and not supported on most platforms)

It can be a cron expression or alias such as hourly, daily, weekly, monthly etc. So we were not so sure what name could be more reasonable.

I think this PR flyteorg/flytekit#188 gives more details.

kumare3 · 2020-09-30T18:33:14Z

minor nit, then +1

wild-endeavor

not sure what @katrogan thinks?

protos/flyteidl/admin/schedule.proto

honnix · 2020-10-01T16:09:59Z

@wild-endeavor I guess it is legacy reason, the cron expression handling is very weird in flytekit as it basically casts from a richer semantics to a weaker one by string manipulation. Is it OK with this change we start accepting Unix flavoured cron in launch plan definition? For example, for the deprecated cron expression we keep the the existing logic unchanged, while for CronSchedule we parse it directly using croniter, and the validation will also allow cron alias, such as daily, weekly, etc. The validation logic can be found in the proposed flytekit PR.

Co-authored-by: Yee Hing Tong <[email protected]>

honnix · 2020-10-01T20:46:32Z

@wild-endeavor What do you think of the name of this class: https://github.com/lyft/flytekit/blob/6274318ad9f64b0367754a917d08d0f133358300/flytekit/common/schedules.py#L128. There is already a class named CronSchedule.

honnix · 2020-10-02T07:09:42Z

I have a second thought now, that I think it might make other changes easier (flytekit and flyteadmin) to name the message as CronScheduleWithOffset, without conflicting with existing names that we need to keep for backward compatibility.

wild-endeavor · 2020-10-02T16:20:19Z

+1 after changing the name to CronSchedule

katrogan · 2020-10-02T16:26:00Z

I like CronSchedule too! I think it will make sense to move to that as the replacement for cron_expression (which like @honnix mentioned is a weird/less-structured format anyways)

honnix · 2020-10-02T16:52:09Z

How do we handle the python class name then? We could have more validation branches to cover alias, Unix cron and existing cron in the same class. How does that sound?

This reverts commit e468ea8.

honnix · 2020-10-02T17:30:06Z

@katrogan @wild-endeavor I revert it back to CronSchedule. Please take a look again. Thanks.

katrogan · 2020-10-02T17:31:34Z

protos/flyteidl/admin/schedule.proto

@@ -16,14 +16,20 @@ message FixedRate {
    FixedRateUnit unit = 2;
 }

+message CronSchedule {
+    string schedule = 1;
+    string offset = 2;


one nit: can you add comments on what the expected format for schedule and offset should be?

@katrogan Do we plan to support AWS cron as well in CronSchedule.schedule? If yes, I can revise the comment a bit.

I think we should move away from the aws cron type (which is more work) and have admin handle converting to it in its aws specific scheduler handler. But that can be a separate issue and we can keep using the deprecated cron_expression in the meantime

I see. The problem is the 5-fields cron syntax is weaker than the one used by AWS, so it's not gonna be an 100% mapping.

katrogan

LGTM, @wild-endeavor mind taking one last look?

honnix · 2020-10-02T19:54:56Z

Alright. I'm merging this and will make changes in flytekit accordingly. Thanks everyone for reviewing and commenting on this PR.

honnix · 2020-10-02T20:04:18Z

@katrogan I drafted this release https://github.com/lyft/flyteidl/releases/tag/untagged-eaee7bcfa18de386fe2b, please take a look and release if it is OK.

katrogan · 2020-10-02T20:07:17Z

@honnix looks good!

honnix · 2020-10-02T20:30:48Z

Somehow the github workflow is not triggered to publish to pypi. 🤔

wild-endeavor · 2020-10-02T20:35:43Z

I just tried to push it manually - can you try to pull on your end?

honnix · 2020-10-02T20:40:38Z

@wild-endeavor Yeah that worked. I guess I messed it by drafting a release and then publish. There are weird things regarding which event to use: https://stackoverflow.com/questions/59319281/github-action-different-between-release-created-and-published

* Extend `CronSchedule` to support `schedule` and `offset` added in flyteorg/flyteidl#83 Co-authored-by: tnsetting <[email protected]>

* Support cron schedule with offset. For example: https://github.com/spotify/styx#schedule-string * Add generated files * Update protos/flyteidl/admin/schedule.proto Co-authored-by: Yee Hing Tong <[email protected]> * Update protos/flyteidl/admin/schedule.proto Co-authored-by: Yee Hing Tong <[email protected]> * Update protos/flyteidl/admin/schedule.proto Co-authored-by: Yee Hing Tong <[email protected]> * Regenerate files * Regenerate files * Change the message name back * Revert "Change the message name back" This reverts commit e468ea8. * Add comments for schedule and offset Co-authored-by: Yee Hing Tong <[email protected]>

honnix requested a review from kumare3 September 29, 2020 09:59

honnix force-pushed the support-styx-schedule branch from 45e696e to 28680fc Compare September 29, 2020 11:25

kumare3 reviewed Sep 29, 2020

View reviewed changes

protos/flyteidl/admin/schedule.proto Outdated Show resolved Hide resolved

Support cron schedule with offset.

58b4964

For example: https://github.com/spotify/styx#schedule-string

honnix force-pushed the support-styx-schedule branch from 28680fc to 58b4964 Compare September 29, 2020 18:10

Add generated files

b8e3bb5

honnix mentioned this pull request Sep 30, 2020

Cron schedule with offset flyteorg/flytekit#188

Merged

8 tasks

kumare3 reviewed Sep 30, 2020

View reviewed changes

wild-endeavor reviewed Oct 1, 2020

View reviewed changes

protos/flyteidl/admin/schedule.proto Outdated Show resolved Hide resolved

protos/flyteidl/admin/schedule.proto Outdated Show resolved Hide resolved

protos/flyteidl/admin/schedule.proto Outdated Show resolved Hide resolved

honnix and others added 4 commits October 1, 2020 22:38

Update protos/flyteidl/admin/schedule.proto

d85bb36

Co-authored-by: Yee Hing Tong <[email protected]>

Update protos/flyteidl/admin/schedule.proto

49af326

Co-authored-by: Yee Hing Tong <[email protected]>

Update protos/flyteidl/admin/schedule.proto

9d8da9e

Co-authored-by: Yee Hing Tong <[email protected]>

Regenerate files

bb82e3e

Regenerate files

bc2358c

Change the message name back

e468ea8

Revert "Change the message name back"

3db00ef

This reverts commit e468ea8.

katrogan reviewed Oct 2, 2020

View reviewed changes

Add comments for schedule and offset

907506e

katrogan approved these changes Oct 2, 2020

View reviewed changes

wild-endeavor approved these changes Oct 2, 2020

View reviewed changes

honnix merged commit 4c06596 into master Oct 2, 2020

honnix deleted the support-styx-schedule branch October 2, 2020 19:55

honnix added a commit to flyteorg/flytekit that referenced this pull request Oct 4, 2020

Cron schedule with offset (#188)

5b5502b

* Extend `CronSchedule` to support `schedule` and `offset` added in flyteorg/flyteidl#83 Co-authored-by: tnsetting <[email protected]>

katrogan mentioned this pull request Dec 15, 2020

[Feature] Use generic cron expressions in FlyteAdmin flyteorg/flyte#641

Closed

13 tasks

max-hoffman pushed a commit to dolthub/flytekit that referenced this pull request May 11, 2021

Cron schedule with offset (flyteorg#188)

5e415de

* Extend `CronSchedule` to support `schedule` and `offset` added in flyteorg/flyteidl#83 Co-authored-by: tnsetting <[email protected]>

pingsutw pushed a commit to pingsutw/flyte-monorepo that referenced this pull request Apr 4, 2023

Cron schedule with offset (#188)

6d36883

* Extend `CronSchedule` to support `schedule` and `offset` added in flyteorg/flyteidl#83 Co-authored-by: tnsetting <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Styx style schedule #83

Support Styx style schedule #83

honnix commented Sep 29, 2020

katrogan commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 30, 2020

kumare3 Sep 30, 2020

honnix Sep 30, 2020

honnix Sep 30, 2020

kumare3 commented Sep 30, 2020

wild-endeavor left a comment

honnix commented Oct 1, 2020

honnix commented Oct 1, 2020

honnix commented Oct 2, 2020

wild-endeavor commented Oct 2, 2020

katrogan commented Oct 2, 2020

honnix commented Oct 2, 2020

honnix commented Oct 2, 2020

katrogan Oct 2, 2020

honnix Oct 2, 2020

katrogan Oct 2, 2020

honnix Oct 2, 2020

katrogan left a comment

honnix commented Oct 2, 2020

honnix commented Oct 2, 2020

katrogan commented Oct 2, 2020

honnix commented Oct 2, 2020

wild-endeavor commented Oct 2, 2020

honnix commented Oct 2, 2020

Support Styx style schedule #83

Support Styx style schedule #83

Conversation

honnix commented Sep 29, 2020

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

katrogan commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 29, 2020

honnix commented Sep 30, 2020

kumare3 Sep 30, 2020

Choose a reason for hiding this comment

honnix Sep 30, 2020

Choose a reason for hiding this comment

honnix Sep 30, 2020

Choose a reason for hiding this comment

kumare3 commented Sep 30, 2020

wild-endeavor left a comment

Choose a reason for hiding this comment

honnix commented Oct 1, 2020

honnix commented Oct 1, 2020

honnix commented Oct 2, 2020

wild-endeavor commented Oct 2, 2020

katrogan commented Oct 2, 2020

honnix commented Oct 2, 2020

honnix commented Oct 2, 2020

katrogan Oct 2, 2020

Choose a reason for hiding this comment

honnix Oct 2, 2020

Choose a reason for hiding this comment

katrogan Oct 2, 2020

Choose a reason for hiding this comment

honnix Oct 2, 2020

Choose a reason for hiding this comment

katrogan left a comment

Choose a reason for hiding this comment

honnix commented Oct 2, 2020

honnix commented Oct 2, 2020

katrogan commented Oct 2, 2020

honnix commented Oct 2, 2020

wild-endeavor commented Oct 2, 2020

honnix commented Oct 2, 2020