dbader / schedule

Python job scheduling for humans.
https://schedule.readthedocs.io/
MIT License
11.73k stars 960 forks source link

Resolves #470: Add timezone functionality #471

Closed chrimaho closed 2 years ago

chrimaho commented 3 years ago

Add timezone functionality.

Key changes:

Resolves #470.

samgermain commented 2 years ago

Are there any plans to finish this?

SijmenHuizenga commented 2 years ago

Hi @chrimaho, thanks for the pr!

Something you might have noticed is that the schedule library has no dependencies and can operate all by itself. This makes the code easy to understand, simple to copy and adjust. If we can it like this that would be pretty cool.

Schedule is built to be a super simple scheduler for simple schedule requirements. Timezones unfortunately are not simple at all. That's why I've said again and again that timezones are out of scope for this project. Let's leave the complexity of timezones to the more advanced scheduling solutions.

But, I like your solution a lot! It is easy to understand and seems pretty fail-safe. I'm thinking that it would be possible to write this timezone functionality as an optional extension to the schedule library. Maybe as a separate library altogether? In that case someone would be able to do pip install schedule-zoned and dependencies would be managed automatically.

This is an untested prototype to preview my thinking:

from typing import Optional

from schedule import (
    Scheduler,
    Job
)
import pytz
import tzlocal

class ZonedJob(Job):

    def __init__(self, interval: int, scheduler: Scheduler = None):
        # Default time zone
        self.at_tz: Optional[str] = tzlocal.get_localzone()

        super(ZonedJob, self).__init__(interval, scheduler)

    def tz(self, timezone:str):
        """
        Specify the timezone that the job should run at.

        :param timezone: The defined timezone for this job.
        :return: The invoked job instance
        """
        if not timezone in pytz.all_timezones:
            raise pytz.UnknownTimeZoneError(
                "Invalid time zone: %s.\nYou can check for valid time zones with: pytz.all_timezones" % (timezone)
            )
        self.at_tz = timezone
        return self

class ZonedScheduler(Scheduler):

    def every(self, interval: int = 1) -> "ZonedJob":
        """
        Schedule a new periodic job.

        :param interval: A quantity of a certain time unit
        :return: An unconfigured :class:`Job <Job>`
        """
        job = ZonedJob(interval, self)
        return job

(To make this prototype work we would have to refactor all datetime.datetime.now occurrences into a function inside the scheduler that, in turn, can be overwritten by the ZonedScheduler)

Another option we could explore is to have it as an optional module inside this project, but I'm not sure how users would need to manage dependencies. More ideas and thoughts are welcome :)

I'm interested to hear what you think

For now I'm going to close this pr as in the current shape it doesn't fit the project. But don't be discouraged to continue the discussion below

rudSarkar commented 2 years ago

Hi @SijmenHuizenga,

Near future timezone will add on schedule module? because sometimes VPS/VPC in other countries so it doesn't match the time local need to write extra code to identify the timezone.

Looking forward to hearing from you.

Thanks

SijmenHuizenga commented 2 years ago

Hi @rudSarkar and @chrimaho, what do you think of #517? Would that help you out? Looking forward to your thought!

chrimaho commented 2 years ago

Hi @dbader and @SijmenHuizenga , I really like this new info within #517 . Especially the optional pytz module and lazy-loading for it's import. I think that this will work really well!

I also think that having a separate module will not be necessary. I understand the prerogative for having the core scheduler as streamlined and simple as possible. But I also think that having Time Zone functionality is a common functionality and should be part of the base product for this library.

Moreover, it becomes particularly apparent when using this library on a server (eg. on AWS), and when the time-zone of the server (eg. Landing Zone) is not the same local timezone of where the Python app will be used. So, it's I think that having this time-zone functionality here in this one library is important. And the suggestion here by @SijmenHuizenga looks very good 👍

Reading through the code on the PR, I think that it is easy enough to resolve those errors raised by PyTest and MyPy. If you Merge #517 in, I am happy to fix it.

samgermain commented 2 years ago

Hi @dbader and @SijmenHuizenga , I really like this new info within #517 . Especially the optional pytz module and lazy-loading for it's import. I think that this will work really well!

I also think that having a separate module will not be necessary. I understand the prerogative for having the core scheduler as streamlined and simple as possible. But I also think that having Time Zone functionality is a common functionality and should be part of the base product for this library.

Moreover, it becomes particularly apparent when using this library on a server (eg. on AWS), and when the time-zone of the server (eg. Landing Zone) is not the same local timezone of where the Python app will be used. So, it's I think that having this time-zone functionality here in this one library is important. And the suggestion here by @SijmenHuizenga looks very good 👍

Reading through the code on the PR, I think that it is easy enough to resolve those errors raised by PyTest and MyPy. If you Merge #517 in, I am happy to fix it.

Or at least be able to use UTC

rudSarkar commented 2 years ago

Hi, @SijmenHuizenga I think it's fine but there are a few errors on testing, and not possible to merge right away, If the issue will be fixed it can be usable with timezone support.

Added a comment on #517 PR about the optional field of the timezone, It's a good idea to make timezone optional.