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

feat(TaskProcessingApi): Add endpoint for getting the next task #45391

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Summary

The ExApps need a way to get the next task.
@marcelklehr I think there is a bug as the tasks are not marked as running when the getNextScheduledTask method of the service is called (or maybe it was also intentional and the endpoint should do it manually).

Checklist

@provokateurin provokateurin added the 2. developing Work in progress label May 17, 2024
@provokateurin provokateurin added this to the Nextcloud 30 milestone May 17, 2024
@marcelklehr
Copy link
Member

Ah, I missed this, yes.

@marcelklehr I think there is a bug as the tasks are not marked as running when the getNextScheduledTask method of the service is called (or maybe it was also intentional and the endpoint should do it manually).

Yeah, let's do this manually directly in the endpoint

@marcelklehr marcelklehr added the pending documentation This pull request needs an associated documentation update label May 18, 2024
@provokateurin provokateurin force-pushed the feat/taskprocessingapi/next-task-endpoint branch from 5800675 to 6d86511 Compare June 4, 2024 10:58
@provokateurin provokateurin added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 4, 2024
@provokateurin provokateurin marked this pull request as ready for review June 4, 2024 11:15
@bigcat88
Copy link
Member

bigcat88 commented Jun 5, 2024

We need endpoint for getting nextTask by specifying multiple task_types.
One provider can process multiple task types and we do not want to spam NC with one nextTask request for each task_type.

Or just to change string $taskTypeId to array of strings.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

  • I agree with @marcelklehr that it makes more sense to change the task status to "running" in the controller method.
    The Manager::getNextScheduledTask method might be used in some context where we don't want to change to task status.

  • Also, how about having a setTaskRunning parameter (with true as default value) in TaskProcessingApiController::getNextScheduledTask ? Just in case one wants to get the next scheduled task without actually processing it.

  • About requesting multiple task types in getNextScheduledTask, much needed indeed. @marcelklehr Would you go with another endpoint or change the current one?

@provokateurin
Copy link
Member Author

I don't see a reason why you would ever need to know the next task without also running it at the same time, but I can change it if you really want 🤷‍♀️

@marcelklehr
Copy link
Member

I don't see a reason why you would ever need to know the next task without also running it at the same time

The synchronous background job will re-schedule itself based on the fact whether there is another job waiting to be executed.

@marcelklehr
Copy link
Member

Would you go with another endpoint or change the current one?

We can add this to the current endpoint.

@provokateurin provokateurin force-pushed the feat/taskprocessingapi/next-task-endpoint branch from 711a3fe to 7e6e94a Compare June 7, 2024 06:55
@provokateurin provokateurin changed the base branch from master to feat/securitymiddleware/bypass-app_api June 7, 2024 06:55
@provokateurin provokateurin force-pushed the feat/taskprocessingapi/next-task-endpoint branch from 7e6e94a to 8063ff5 Compare June 7, 2024 09:35
@provokateurin provokateurin changed the base branch from feat/securitymiddleware/bypass-app_api to master June 7, 2024 09:35
@provokateurin provokateurin force-pushed the feat/taskprocessingapi/next-task-endpoint branch from c2caae2 to d64aa3a Compare June 17, 2024 09:34
@bigcat88
Copy link
Member

ExApp needs to call these APIs to process tasks:

getNextScheduledTask
getFileContents
setProgress
setResult

more precisely, there should be two versions of getFileContents - one for the UI with userId and other one for ExApp with [ExAppRequired] without checking for userId

cause only having those 4 endpoint ExApp can have no "system" flag to process tasks from what I see

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

The security middleware looks good now

…e.php

Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
…ns with `[ExAppRequired]`

Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
@bigcat88
Copy link
Member

bigcat88 commented Jun 28, 2024

I didn't change the logic inside getNextScheduledTask - but it doesn't seem right to me for these reasons:

  1. $task->setStatus(Task::STATUS_RUNNING); won't write anything to the database, right?
  2. Reading and writing to the database must be atomic within “getNextScheduledTask” (so that two requests for processing a task in parallel do not receive the same task)
  3. I didn’t understand the logic at all why $providerIds is there as input and what it should do, that’s why I didn’t change the entry to atomic inside “getNextScheduledTask”

throw new NotFoundException();
}

$task->setStatus(Task::STATUS_RUNNING);
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, good point @bigcat88, this needs to be persisted


/**
* Returns the next scheduled task for the taskTypeId
*
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
*
*
* It could be that a provider is looking for new tasks but the task type of the next scheduled task is actually configured to be run with a different provider

$task = $this->taskProcessingManager->getNextScheduledTask($taskTypeIds);

$provider = $this->taskProcessingManager->getPreferredProvider($task->getTaskTypeId());
if (!in_array($provider->getId(), $providerIds, true)) {
Copy link
Member

Choose a reason for hiding this comment

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

Imagine the task queue is as follows

1. free_prompt
2. summarize

and the configuration for the preferred providers says

free_prompt: context_chat
summarize: llm2-llama-3

Then llm2 calls this endpoint with getNextScheduledTask([free_prompt, summarize], [llm2-llama-3])

What will happen is:

  • getNextScheduledTask will return the free_prompt task
  • But that is configured to be run by context_chat
  • So, the endpoint returns NotFoundResponse, even though there is the summarize task waiting that fulfills the requirements of the request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants