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: TaskProcessing API #45094

Merged
merged 63 commits into from
May 15, 2024
Merged

feat: TaskProcessing API #45094

merged 63 commits into from
May 15, 2024

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Apr 29, 2024

Summary

TODO

  • Clean up old tasks
  • Clean up old app data files
  • Add integration tests
  • Check if user has access to files
  • OCS API

Checklist

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
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.

Yay, it looks gooood. Didn't make a full review yet, just using it for the work on the assistant.
It breaks NC though. I tried to rebase on master, no luck.

PHP Fatal error:  Class OC\\Files\\Node\\LazyR
oot contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (OCP\\Files\\IRootFolder::getAppDataDirectoryName) in /var/www/html/dev
/server/lib/private/Files/Node/LazyRoot.php on line 39

@marcelklehr marcelklehr added 2. developing Work in progress pending documentation This pull request needs an associated documentation update labels May 3, 2024
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.

A bump of the server version is needed to trigger the migration step.

lib/public/TaskProcessing/Task.php Show resolved Hide resolved
@marcelklehr
Copy link
Member Author

A bump of the server version is needed to trigger the migration step.

Like this?

version.php

-$OC_Version = [30, 0, 0, 0];
+$OC_Version = [30, 0, 0, 1];

@marcelklehr marcelklehr marked this pull request as ready for review May 6, 2024 09:10
@marcelklehr marcelklehr requested a review from come-nc May 6, 2024 09:11
core/ResponseDefinitions.php Outdated Show resolved Hide resolved
core/ResponseDefinitions.php Outdated Show resolved Hide resolved
core/ResponseDefinitions.php Outdated Show resolved Hide resolved
core/ResponseDefinitions.php Show resolved Hide resolved
core/ResponseDefinitions.php Outdated Show resolved Hide resolved
core/Controller/TaskProcessingApiController.php Outdated Show resolved Hide resolved
core/Controller/TaskProcessingApiController.php Outdated Show resolved Hide resolved
core/Migrations/Version30000Date20240429122720.php Outdated Show resolved Hide resolved
lib/composer/composer/autoload_static.php Show resolved Hide resolved
lib/private/AppFramework/Bootstrap/RegistrationContext.php Outdated Show resolved Hide resolved
lib/private/AppFramework/Bootstrap/RegistrationContext.php Outdated Show resolved Hide resolved
lib/private/TaskProcessing/Db/Task.php Outdated Show resolved Hide resolved
lib/private/TaskProcessing/Manager.php Show resolved Hide resolved
core/Controller/TaskProcessingApiController.php Outdated Show resolved Hide resolved
core/Controller/TaskProcessingApiController.php Outdated Show resolved Hide resolved
core/ResponseDefinitions.php Outdated Show resolved Hide resolved
lib/private/TaskProcessing/Db/Task.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
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.

Another partial review:

  • We could use (optional) default values for input shape fields. It might not make sense in many cases but for example: "number of images" for TextToImage could have 1 as default value. Wdyt?
  • The assistant needs a manager method AND/OR an endpoint to get all user's tasks (with a task type ID optional filter). Wdyt?
  • [minor] CSRF might not be needed for taskprocessing/tasks/{taskId}/file/{fileId}, not a big deal, I currently add the requesttoken GET param to display images in the UI.
  • Task result files stored in data/appdata_iid/core/TaskProcessing/ are empty for the TextToImage task type (at least). I tried 2 providers (OpenAI and Replicate which are providers for the old TTI API).
  • The manager is unable to read input files if they are in a user's storage. I tried an STT provider (for the old STT API), I get this exception raised: https://github.com/nextcloud/server/blob/enh/taskprocessing-api/lib/private/TaskProcessing/Manager.php#L763 which means this does not get the file: https://github.com/nextcloud/server/blob/enh/taskprocessing-api/lib/private/TaskProcessing/Manager.php#L759 . Happy to debug that together if you want.

@marcelklehr
Copy link
Member Author

  • The assistant needs a manager method AND/OR an endpoint to get all user's tasks fixed ✔️
  • [minor] CSRF might not be needed for taskprocessing/tasks/{taskId}/file/{fileId} fixed ✔️
  • Task result files stored in data/appdata_iid/core/TaskProcessing/ are empty for the TextToImage task type. fixed ✔️

marcelklehr and others added 22 commits May 14, 2024 11:38
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: julien-nc <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: julien-nc <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Wasn't able to load File from app data

Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: Kate <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
@marcelklehr marcelklehr merged commit f3e72af into master May 15, 2024
157 checks passed
@marcelklehr marcelklehr deleted the enh/taskprocessing-api branch May 15, 2024 09:43
* @return string
* @since 30.0.0
*/
public function getAppDataDirectoryName(): string;
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants