Schedule timezone fixes #3315
Schedule timezone fixes #3315
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
internal
src/zenml/config/schedule.py
Outdated
| logger.warning( | ||
| "Your schedule `%s` is missing a timezone. It will be treated " | ||
| "as a datetime in your local timezone." | ||
| ) |
There was a problem hiding this comment.
| logger.warning( | |
| "Your schedule `%s` is missing a timezone. It will be treated " | |
| "as a datetime in your local timezone." | |
| ) | |
| logger.warning( | |
| f"Your schedule `{info.field_name}` is missing a timezone. It will be treated " | |
| "as a datetime in your local timezone." | |
| ) |
There was a problem hiding this comment.
https://google.github.io/styleguide/pyguide.html#3101-logging
For logging functions that expect a pattern-string (with %-placeholders) as their first argument: Always call them with a string literal (not an f-string!) as their first argument with pattern-parameters as subsequent arguments.
No :)
There was a problem hiding this comment.
We're sort of following this style guide for our code base, but even more so, the argument of filtering log messages makes a lot of sense I think. Right now we're just logging to stdout, but we've had multiple discussions already and I'm sure at some point we will allow customizing some logging backends where ZenML logs to. So we should just write our logs with these placeholders, to be prepared for when (not if) this is being implemented.
There was a problem hiding this comment.
Ok, Michael, but you're still missing the argument, heee heee
There was a problem hiding this comment.
Yep that I fixed, thanks 🙏
There was a problem hiding this comment.
"Your schedule %s is missing a timezone" comment is missing a schedule 😄
Describe changes
This PR contains some fixes to the timezones when using schedules:
The user-facing
Scheduleobject that is used to define schedules will treat any timezone unaware datetime as if it was in the local timezone. This enables users to write something like thismy_pipeline.with_options(Schedule(start_time=datetime.now())which will create a schedule that starts immediately. To have more control over it, theScheduleobject also allows users to pass timezone aware datetime, which will respect the given timezone.The
ScheduleRequestobject behaves a little differently: When it is passed a timezone aware datetime, it converts it to UTC timezone and then makes it timezone unaware. If it is passed a naive datetime, it simply assumes it is in UTC time. This means when we create this object internally as part of a pipeline run, we already have timezone aware datetimes and convert them to UTC. If someone were to externally call thePOST /schedulesendpoint and they pass naive datetimes, we treat them as UTC similar to all other endpoints.Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes