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

AndTrigger does not work #281

Closed
NikkoFox opened this issue Jan 25, 2018 · 17 comments
Closed

AndTrigger does not work #281

NikkoFox opened this issue Jan 25, 2018 · 17 comments
Milestone

Comments

@NikkoFox
Copy link

NikkoFox commented Jan 25, 2018

Hi
Here is part of the code, but the scheduler does not work
And the example from the documentation also does not work.

from apscheduler.schedulers.blocking import BlockingScheduler
from apscheduler.triggers.interval import IntervalTrigger
from apscheduler.triggers.cron import CronTrigger
from apscheduler.triggers.combining import AndTrigger
scheduler = BlockingScheduler()
trigger_day = AndTrigger([IntervalTrigger(minutes=2), CronTrigger(hour='7-22')])
scheduler.add_job(job, trigger_day, max_instances=3, id='test')
scheduler.replace_existing = True
scheduler.start()

What am I doing wrong?

@agronholm agronholm added the bug label Jan 25, 2018
@agronholm
Copy link
Owner

It seems that this AndTrigger can never find a next fire time. This is a bug.

@agronholm
Copy link
Owner

Disregard that. The actual problem is that because IntervalTrigger starts counting from current time, it never agrees with the CronTrigger because CronTrigger never produces any nonzero seconds or microseconds that IntervalTrigger likely will. This problem needs to be documented and tools provided to debug triggers that hang this way.

@alexdev27
Copy link

Any news about that problem?

@agronholm
Copy link
Owner

I've explained the problem and it's a design issue. If you have any suggestions, I'm open to ideas.

@kaos
Copy link

kaos commented Jun 25, 2019

First thing that comes to my mind, is to have an optional parameter threshold that indicates how close the times has to be (in seconds, for instance) in order to be accepted.
Example:

AndTrigger([IntervalTrigger(minutes=2), CronTrigger(hour='7-22')], threshold=2)

So if the difference between the interval and cron next run times are less than 2 seconds, use one of them. (like, the closest to occur, or the one from the first trigger in the list, etc..)

@agronholm
Copy link
Owner

I am adding a threshold parameter to the trigger as suggested. It will be in the next 4.0 series release.

@agronholm agronholm added this to the 4.0 milestone Jul 23, 2019
@agronholm
Copy link
Owner

@kaos Trying to implement this, I realized that there is a potential problem: triggers like the interval trigger may veer "off course" if threshold is not 0 because the previous fire time for the next iteration might be a "compromise" between several triggers. Do you feel like this will be a problem? The "real" solution would be storing the "previous fire times" generated by all triggers but I am not at the point where I would consider stateful triggers as a feature.

@kaos
Copy link

kaos commented Aug 13, 2019

How's that?
The trigger time ought to even out to get synchronized. Ah, it all comes down to how the trigger time to use is selected.
I'd say this would be "by design". With stateless triggers (as we want them) we only need to be clear how the trigger time is picked, when using a threshold.

I see a few options here:

a. Use the trigger time from the first trigger in the list. Always. If this is the interval trigger, it may drag, as you described. But if you specify the cron trigger first, then the interval trigger, you should be fine, as the interval trigger will get a "clean" even last trigger time, that we got from the cron trigger, and as such, will not veer off.

b. An additional option specifying which trigger (index) to be "normative" (which might default to the first, for instance)

c. Give the triggers a sense of "priority", which dictates which triggers time to actually use. Possibly as input parameter to the trigger itself, with a default for cron trigger to have a higher prio than interval. This would be the most flexible I think, but might be overkill.

@agronholm
Copy link
Owner

agronholm commented Aug 13, 2019

A bit after I wrote my previous comment I ended up attempting to fix this just like your first suggestion but I haven't got it to work yet.

@thor-the-norseman
Copy link

thor-the-norseman commented May 31, 2020

@agronholm Speaking of priorities, I have a use case for something like that:

I'm working on a project named ZASD (ZFS Automatic Snapshot Daemon) in which I have a set of cron schedules, each of which trigger filesystem snapshots. In a typical configuration, a group of schedules A, B, C, ..., will trigger snapshots in a repeating lockstep pattern, say ABC, B, BC, B, ABC, B, BC, B, ...

Crucially, each schedule is associated with a user-configured tag (such as 'weekly', 'daily' or 'hourly') a list of filesystems and a priority. When triggers coincide on the same filesystem, the trigger with the highest priority is picked and the rest are discarded, since only one snapshot is required at a time. Having redundant snapshots pollutes the system-wide snapshot list.

I have faced great difficulties in applying APScheduler to this use case without resorting to monkey patching your code.

Could you please:

a) Promise that coinciding triggers on an OrTrigger are picked in the order they were listed in, and provide a means for a job to discover which trigger object was picked?

b) Provide a means for a job to discover the full chain of triggers that caused it to execute?

Option A is simpler to implement, but isn't useful if the full trigger traversal path is significant to your application. Option B is more general and doesn't require any guarantees.

Discovery of the concrete Python objects isn't important. I would rather have a means of attaching generic metadata to jobs, triggers and schedules, and a means of retrieving it from inside an executing job.

@agronholm
Copy link
Owner

I have faced great difficulties in applying APScheduler to this use case without resorting to monkey patching your code.

Sounds like it might be easier for you to write your own trigger class then?

b) Provide a means for a job to discover the full chain of triggers that caused it to execute?

I had not intended to do that, as I don't have any idea how to fit that into the planned API (the combining triggers aren't special in that sense). While I don't think this is be what you were thinking of, there are plans to add tracing capabilities to triggers so you can see how their internal decision making works.

By the way, the major refactoring of the triggers for APScheduler 4.0 is done. The rest of the library does not work yet, but the triggers pass all the tests with 100% line coverage. Feel free to experiment!

@agronholm
Copy link
Owner

There is a threshold parameter for AndTrigger in 4.0, and the use case should've used CronTrigger(hour='7-22', minute='*/2') anyway.

@rafalkrupinski
Copy link

Wouldn't it be easier to add a param to IntervalTrigger to make it round the next fire time to a full second?

@agronholm
Copy link
Owner

You mean add special casing for IntervalTrigger? Then what about all the custom triggers people could have on their own?

@agronholm
Copy link
Owner

What I mean to say is, do you have a problem with the solution I came up with?

@rafalkrupinski
Copy link

rafalkrupinski commented Oct 2, 2023

I don't like the threshold, because it's not obvious what happens when threshold kicks in - is the earliest time used, latest or average? I need to check the documentation. Should the next fire time be calculated from the last actual fire time or from the last result of Interval.next()? There are no options to control these behaviors. Threshold seems very complex to implement right.

On the other hand, we already have all the tools to make it work - setting first fire time to the full second and not using µs in the interval.
Optionally there could be round-to-second parameter, that would save user from calculating the first fire time.

You mean add special casing for IntervalTrigger?

Interval is special case anyway, as it's the only built-in trigger with µs resolution, isn't it?

Then what about all the custom triggers people could have on their own?

If you want to combine your triggers in 3.x branch they already have to round fire times.
4.x is alpha - anything goes.


edit:
Ah, it's actually about alignment than resolution. My bad.

Still, all other (built-in) triggers align to full seconds don't they? So to combine any other trigger with them, they need to be second-aligned too. In general it should be users responsibility to align their triggers, but there could be an option to auto-align IntervalTrigger to full second.

Am I missing something?

@agronholm
Copy link
Owner

The threshold option is a substantially more flexible solution than rounding to the nearest second, as you can set a higher or lower threshold than one second.

Are you unsatisfied with how the combining triggers are documented (here)?

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

No branches or pull requests

6 participants