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

Cancelled Occurrence replaced by a new Event/Occurrence causes occurrences_after to fail #562

Open
dwasyl opened this issue Apr 11, 2024 · 2 comments

Comments

@dwasyl
Copy link

dwasyl commented Apr 11, 2024

Where there is an Event with a cancelled Occurrence, but another Event has been scheduled in place of the original at the same time, it causes EventListManager.occurrences_after() to fail at next_occurrence = heapq.heappop(occurrences)[0] with an '<' not supported between instances of 'generator' and 'generator' error.

Part of the issue seems to be that EventListManager.occurrences_after() doesn't employ the same logic as Event.get_occurrences() to work persisted occurrences into the generator set.

For example, in a case where an Event has only a single Occurrence and that Occurrence is cancelled, how should that be treated here? In either case, it causes occurrences_after() to fail which isn't the correct behaviour.

The use of OccurrenceReplacer happens at the end, but it has already failed due to match Occurrences being added to the heap.

@dwasyl
Copy link
Author

dwasyl commented Apr 11, 2024

Continuing to investigate my own issue, the problem is linked to the heaps PriorityQueue implmentation as described https://docs.python.org/3/library/heapq.html#priority-queue-implementation-notes

Based on some notes from here:
https://stackoverflow.com/questions/56998340/heapq-cant-handle-tuples-having-same-priority-if-the-item-is-not-comparable, the simplest would be to add an occurrent/event # to the tuple that gets added to the heap, however, it would be difficult to find that again based on the use of heapreplace in schedule/utils.py:

            try:
                next_occurrence = heapq.heapreplace(
                    occurrences, (next(generator), generator)
                )[0]

Potentially just added the Event.pk to the tuple would deal with the issue as a single Event likely wouldn't get a duplicate Occurrence, the is easily replicable when doing the heapreplace.

Thoughts?

@dwasyl
Copy link
Author

dwasyl commented Apr 11, 2024

For the record, having posted this the solutions became easier to solve, the EventListManager.occurrences_after() needed some tweaking to add an occurrence count to the heap tuple to that the generators don't get compared.

On a very simple basis:

    def occurrences_after(self, after=None):
        """
        It is often useful to know what the next occurrence is given a list of
        events.  This function produces a generator that yields the
        the most recent occurrence after the date ``after`` from any of the
        events in ``self.events``
        """
        from schedule.models import Occurrence

        if after is None:
            after = timezone.now()
        occ_replacer = OccurrenceReplacer(
            Occurrence.objects.filter(event__in=self.events)
        )
        generators = [
            event._occurrences_after_generator(after) for event in self.events
        ]
        occurrences = []

        occ_count = 1
        for generator in generators:
            try:
                heapq.heappush(occurrences, (next(generator), occ_count, generator))
            except StopIteration:
                pass
            occ_count += 1

        while occurrences:
            generator = occurrences[0][2]
            occ_count = occurrences[0][1]

            try:
                next_occurrence = heapq.heapreplace(
                    occurrences, (next(generator), occ_count, generator)
                )[0]
            except StopIteration:
                next_occurrence = heapq.heappop(occurrences)[0]
            yield occ_replacer.get_occurrence(next_occurrence)

The only additions being the three occ_count lines. Perhaps I'll work this into a PR, if you are still looking at updating/making releases @llazzaro ?

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

No branches or pull requests

1 participant