support timezone when scheduling with Argo workflows#1250
support timezone when scheduling with Argo workflows#1250savingoyal merged 10 commits intoNetflix:masterfrom
Conversation
| return " ".join(schedule.schedule.split()[:5]) | ||
| return None | ||
|
|
||
| def _timezone(self): |
There was a problem hiding this comment.
small nit - can we have _cron(...) return a tuple that includes the timezone as well. that will negate the need for creating this function.
|
How do we want to handle AWS Batch, which as far as I can tell, doesn't support a timezone with the cron schedule? It seems like we should at least issue a warning, perhaps even fail, if the user specifies a timezone and we can't use it with it. |
|
Thanks a lot for adding this! Other than addressing the review comment above, can you also add a note about the testing you did for this change? The basic tests to do here would include running a flow providing a timezone and running a flow without a timezone (current behavior). |
|
Yes, I tested by adding a schedule decorator to a simple flow. I created workflows with two different timezones, and then one with no timezone, and confirmed that they all started at the correct time. |
Yep - within AWS Step Functions we would want to check if the user has specified timezone and fail hard if that is the case. Can you also add a note in |
|
cc @valayDave re: impact on Airflow integration |
|
Added documentation for timezone format (IANA standard) and an assertion to check that a timezone isn't provided with Step Functions. |
shrinandj
left a comment
shrinandj
left a comment
There was a problem hiding this comment.
LGTM! Thanks for your contributions!
| def _cron(self): | ||
| schedule = self.flow._flow_decorators.get("schedule") | ||
| if schedule: | ||
| assert ( |
There was a problem hiding this comment.
rather than the assert, can you raise StepFunctionsException so that Metaflow can pretty print the exception?
savingoyal
left a comment
savingoyal
left a comment
There was a problem hiding this comment.
LGTM! (pending a review from @valayDave regarding Airflow compatibility)
| "weekly": False, | ||
| "daily": True, | ||
| "hourly": False, | ||
| "timezone": None, |
There was a problem hiding this comment.
This default value of timezone should be added to the doc string at line number 20. We can even add this link for people to easily know what the format looks like
|
Based on the Airflow docs, I think the way to make this work with Airflow would be to use a timezone-aware metaflow/metaflow/plugins/airflow/airflow.py Line 649 in f6c6320 |
|
@valayDave Should we at least add a check, analogous to the Step Functions check, to fail if the user specifies a timezone with Airflow? |
4 participants
This PR adds support for specifying a timezone when scheduling Argo workflows.
The relevant Argo documentation is here: https://argoproj.github.io/argo-workflows/cron-workflows/#cronworkflow-options