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

Allow create schedule entries from dict #175

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

semigodking
Copy link

This commit is to allow creating RedBeatScheuleEntry directly from data loaded from config files. The reason is that I would like to load static beat schedules for Celery from a YAML/JSON config file. Without this commit the following exception is raised.

redbeat/schedulers.py:209: in __init__
    args=args, kwargs=kwargs, **clsargs)
../venv/lib/python3.6/site-packages/celery/beat.py:127: in __init__
    self.schedule = maybe_schedule(schedule, relative, app=self.app)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

s = {'__type__': 'crontab', 'day_of_month': '*', 'hour': '*', 'minute': '*/5'}, relative = False, app = <Celery celery.tests at 0x7fadb42438>

    def maybe_schedule(s, relative=False, app=None):
        """Return schedule from number, timedelta, or actual schedule."""
        if s is not None:
            if isinstance(s, numbers.Number):
                s = timedelta(seconds=s)
            if isinstance(s, timedelta):
                return schedule(s, relative, app=app)
            else:
>               s.app = app
E               AttributeError: 'dict' object has no attribute 'app'

../venv/lib/python3.6/site-packages/celery/schedules.py:667: AttributeError

@sibson
Copy link
Owner

sibson commented Oct 2, 2020

Thanks for the PR. Could you help me understand what is happening here. There seems to be a missing piece in where does the YAML get parsed into a dict?

@semigodking
Copy link
Author

semigodking commented Oct 2, 2020

hi @sibson,
If Celery config is loaded from Python file, the value of schedule accepted by Celery could be number, time delta and instance of BaseSchedule.
My application initializes Celery with configs loaded from YAML. No additional processing to beat configs. The beat config looks like below:

beat_schedule:	
    "crontab_task":
        "task": "tasks.add"
        "schedule":
              "__type__": "crontab"
              "minute": "*/5"
              "hour": "*" 
        "args":
        - 16
        - 16

As result, when setting up schedules with this config, RedBeatSchedulerEntry is being created with parameter 'schedule' as dict. During initialization of RedBeatSchedulerEntry, it invokes its base class ScheduleEntry's __init__() which uses maybe_schedule to initialize schedule entries.

def maybe_schedule(s, relative=False, app=None):
    """Return schedule from number, timedelta, or actual schedule."""
    if s is not None:
        if isinstance(s, numbers.Number):
            s = timedelta(seconds=s)
        if isinstance(s, timedelta):
            return schedule(s, relative, app=app)
        else:
            s.app = app
    return s

As in my case, the schedule passed in is a dict which is not supported by Celery, exception is raised. So, the solution is letting RedBeatScheduleEntry to create schedule instance before passing to its base class. This PR allows RedBeatScheduleEntry be created from dict. This PR also allows RedBeat to support types of schedules not supported by Celery in future.

@sibson
Copy link
Owner

sibson commented Oct 14, 2020

I'm not sure diverging from upstream Celery is desirable in this case, it creates additional support/maintenance burden for limited gain. This feels like something that can easily be handled within the users app code and I don't see a large number of people clamouring for this as a feature. I'm happy to be proven wrong, but unless larger support exists for this feature or I've misunderstood I don't expect to merge this PR.

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

Successfully merging this pull request may close these issues.

2 participants