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.

Sign up for GitHub

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

Jump to bottom

feat: user facing task queues #25138

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from
Draft

feat: user facing task queues #25138

wants to merge 17 commits into from
+758 −3

Conversation

akhilnarang
Copy link
Member

WIP

Resolves #23008

@akhilnarang akhilnarang force-pushed the user-facing-task-queues branch 2 times, most recently from 82af7f5 to 0e41468 Compare February 29, 2024 12:27
@akhilnarang akhilnarang mentioned this pull request Mar 12, 2024
Merged
This was referenced Mar 13, 2024
Merged
Merged
@stale stale bot added the inactive label Mar 14, 2024
@frappe frappe deleted a comment from stale bot Mar 15, 2024
@stale stale bot closed this Mar 19, 2024
@ankush ankush reopened this Mar 19, 2024
@stale stale bot removed the inactive label Mar 19, 2024
@stale stale bot removed the inactive label Mar 19, 2024
@akhilnarang akhilnarang removed the hotfix label Apr 2, 2024
@akhilnarang akhilnarang force-pushed the user-facing-task-queues branch 3 times, most recently from 8d710ac to f29e739 Compare April 12, 2024 09:27
@stale Stale
Copy link

stale bot commented May 11, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label May 11, 2024
@akhilnarang akhilnarang force-pushed the user-facing-task-queues branch 3 times, most recently from 5b1a251 to dc0a2b2 Compare May 21, 2024 06:29
@akhilnarang
feat: add in a method to convert a callable to the equivalent string
59a3991 
Basically a combination of callable.__module__ + . + callable.__qualname__

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: add in a function to create a notification log entry
3a90a9d 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: add in Background Task doctype
29eef40 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: add in an API endpoint to enqueue a task, and stop a task
1271428 
Make use of `rq.Job`'s `meta` field instead of relying on
`kwargs` -> `Job._kwargs` - doesn't seem to be reliable when a job is
killed

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: add in a button to stop a task
e0c4622 
Cleanup some code
Set status to started when starting

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: check that the method exists before trying to enqueue
128b6a5 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
chore: set task ID in frappe.job
c7a7754 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: show status in list
4ebb32b 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: basic task progress in doc view
257ec11 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: allow retrying
bc57203 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
fix: add in a fallback to callbacks
36d11f4 
Ensure that the status is set even if an exception occurs

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
chore: drop event, add original_task field
0701189 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
fix(request): handle HTTP 201
291767b 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: improve retry task
540767f 
Display a dialog, allow users to customize the values before retrying

Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
fix: don't publish task progress to all users
6af31ef 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
feat: show task runtime
93dd647 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
@akhilnarang
chore: mark as beta; update sort order to match other doctypes
dabca64 
Signed-off-by: Akhil Narang <me@akhilnarang.dev>
ankush
ankush reviewed May 28, 2024
View reviewed changes
frappe/utils/background_jobs.py
if task_id:
frappe.db.set_value(
"Background Task",
{"task_id": task_id.split("::")[-1]},
Copy link
Member

Choose a reason for hiding this comment

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

Lets avoid site name here. Should be possible.

Copy link
Member

Choose a reason for hiding this comment

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

Also move this up so before_job doesn't get commited.

ankush
ankush reviewed May 28, 2024
View reviewed changes
frappe/__init__.py
:param message: Message for the user
:return: Nothing
"""
from frappe.core.doctype.background_task.background_task import publish_task_progress
Copy link
Member

Choose a reason for hiding this comment

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

If we want to make this an API (and other functions in the same file) it should probably be moved / re-exported somewhere else.

ankush
ankush reviewed May 28, 2024
View reviewed changes
frappe/core/doctype/background_task/background_task.json
],
"fields": [
{
"fieldname": "task_id",
Copy link
Member

Choose a reason for hiding this comment

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

Can just use name for this?

ankush
ankush reviewed May 28, 2024
View reviewed changes
frappe/core/doctype/background_task/background_task.py
frappe.msgprint(_("Job is not running."), title=_("Invalid Operation"))


@frappe.whitelist(methods=["POST"])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@frappe.whitelist(methods=["POST"])

ankush
ankush reviewed May 28, 2024
View reviewed changes
frappe/core/doctype/background_task/background_task.json
"read_only": 1
},
{
"fieldname": "stopped_callback",
Copy link
Member

Choose a reason for hiding this comment

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

lets remove all the callbacks. No one uses it afaik. Also it's elegant to handle this stuff in job itself.

@ankush
Copy link
Member

ankush commented May 28, 2024

Dev facing APIs that this should have:

Queuing the task:

  1. Enqueue task and get task ID to track
  2. Automatically subscribe realtime on client

While running the task:

  1. Publish progress (numerical progress with description)
  2. Publish status (?)
  3. Set artefacts or success URL / "Actions"

@ankush
Copy link
Member

ankush commented May 28, 2024

We should auto-subscribe all ongoing tasks so updates are sent even if user reloads the page.

E.g. prepared report completion should be notified ALWAYS.

Another case to consider:

  • User enqueued
  • job started, user left
  • job completed
  • user came back
  • We should notify about the completed job now

This should behave like notification

@stale Stale
Copy link

stale bot commented Jun 13, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankush ankush ankush left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User facing task queues
2 participants
@akhilnarang @ankush