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

Delete and retry tasks through Django Admin #92

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mgax
Copy link
Contributor

@mgax mgax commented Aug 1, 2024

I found it super useful to be able to delete and to reschedule tasks through the Django Admin.


Update – this PR now adds two admin actions:

  • Reenqueue: change the status of the selected tasks to NEW, so they will be executed again.
  • Duplicate: clone the selected tasks, leaving the old ones unchanged, and the new ones as NEW, with no result or started/finished timestamps.

from .models import DBTaskResult


def retry(modeladmin: "DBTaskResultAdmin", request, queryset):
rows = queryset.update(status=ResultStatus.NEW)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RealOrangeOne I'm not sure if it's a good idea to reuse DBTaskResult rows; maybe we should be creating new rows instead. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my use case (processing a bunch of documents), I find it useful to be able to retry failed tasks, until there are none left, fixing errors in the process. If we were to create new rows, then I'd have to stop the queue, retry the tasks, delete the old tasks, and then restart the queue.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Perhaps "Reenqueue" which updates the task, and a "Duplicate" which duplicates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we shall have both! I've updated the PR as you suggest, with a small test to make sure that duplicate tasks actually work.

@mgax mgax force-pushed the feature/admin-delete-and-retry branch from faae1df to a608fc1 Compare August 2, 2024 15:42
@mgax mgax requested a review from RealOrangeOne August 2, 2024 15:45
@Alwinator
Copy link

Alwinator commented Dec 9, 2024

@RealOrangeOne Any plans of merging this soon? We would need this. :)

@RealOrangeOne
Copy link
Owner

@Alwinator it's a draft PR with failing tests and conflicts. It's not as simple as just "merging" it I'm afraid. Once it's ready, it'll absolutely be merged.

@Alwinator
Copy link

Thank you! @mgax Any plans to finish your request? :)

@mgax
Copy link
Contributor Author

mgax commented Dec 12, 2024

Sure, I'd forgotten about this, I'll see about rebasing and fixing the tests.

@mgax mgax force-pushed the feature/admin-delete-and-retry branch from 9125545 to dfb443e Compare December 23, 2024 15:32
@@ -167,6 +167,7 @@ def task_result(self) -> "TaskResult[T]":

object.__setattr__(task_result, "_exception_class", exception_class)
object.__setattr__(task_result, "_traceback", self.traceback or None)
object.__setattr__(task_result, "_return_value", self.return_value)
Copy link
Contributor Author

@mgax mgax Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I rebased my changes, I noticed that this assertion was failing:

assert result_1.return_value == result_2.return_value

I'm pretty sure this used to work, and it looks like the database backend is not fetching return values. Did I find a bug? I'm happy to write a specific test for checking that return_value is set on the task result.

@mgax
Copy link
Contributor Author

mgax commented Dec 23, 2024

I've rebased the branch. I'm not sure why the CI is failing now. I've narrowed it down to this change: mgax@36d3d38. It looks like the failures are flaky, and I'm guessing that the test somehow causes other tests to fail. But I can't reproduce it locally; for me (MacOS 15.1.1, Python 3.13, Django 5.0) just test reliably works.

@@ -226,3 +227,13 @@ def set_failed(self, exc: BaseException) -> None:
"traceback",
]
)

def duplicate(self) -> Self:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This API feels like one better suited for the TaskResult, rather than being backend specific.

@RealOrangeOne RealOrangeOne added the database-backend Issues relating to the database backend label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-backend Issues relating to the database backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants