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

Add rudimentary copywriter functionality, implement assistant metatask wrapper #34

Merged
merged 31 commits into from
Feb 1, 2024
Merged

Add rudimentary copywriter functionality, implement assistant metatask wrapper #34

merged 31 commits into from
Feb 1, 2024

Conversation

MB-Finski
Copy link
Contributor

@MB-Finski MB-Finski commented Jan 29, 2024 •
edited
Loading

This pr implements rudimentary copywriter functionality for assistant.

  • Uses a free prompt provider in the background
  • Implements an assistant meta-task wrapper to differentiate between plain free prompt tasks and custom internal task types
  • Generalizes the wrapper for all current task modalities to unify the notification system.
  • Implements a text file picker for all assistant task types in the front-end. Currently supports ODT, DOC, DOCX, RTF, MD and plain text by using PhpOffice/PhpWord and parsedown to parse the files at the backend.

Known issues:

  • Some psalm issues still remain
  • The UI is not ideal but possibly "enough for an MVP".

@MB-Finski MB-Finski removed the request for review from marcelklehr January 29, 2024 10:34
@MB-Finski
Copy link
Contributor Author

Oh, and task "modality" has not yet been renamed: I'm open for suggestions!

@julien-nc
Copy link
Member

Suggestions:

  • family
  • category
  • group
  • taskOcpNamespace
  • feature

@julien-nc
Copy link
Member

I feel like there are some files missing?

@marcelklehr Which ones?

@MB-Finski
Fix a typo in UI
e579ad5 
Signed-off-by: MB-Finski <[email protected]>
@marcelklehr
Copy link
Member

I feel like there are some files missing?

@marcelklehr Which ones?

Nevermind, may have been a Github glitch

marcelklehr
marcelklehr reviewed Jan 29, 2024
View reviewed changes
lib/Db/Task.php Outdated Show resolved Hide resolved
julien-nc
julien-nc requested changes Jan 31, 2024
View reviewed changes
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.

Huge work 👍
I reviewed half of the PR approximately. The rest is coming this afternoon.

appinfo/routes.php Outdated Show resolved Hide resolved
lib/AppInfo/Application.php Outdated Show resolved Hide resolved
lib/Db/TaskMapper.php Outdated Show resolved Hide resolved
lib/Cron/CleanupAssistantTasks.php Outdated Show resolved Hide resolved
lib/Controller/AssistantController.php Outdated Show resolved Hide resolved
lib/Controller/AssistantController.php Show resolved Hide resolved
lib/Controller/FreePromptController.php Outdated Show resolved Hide resolved
lib/Listener/SpeechToText/SpeechToTextResultListener.php Outdated
Comment on lines 60 to 71
$tasks = $this->taskMapper->getTasksByOcpTaskIdAndCategory($file->getId(), Application::TASK_GATEGORY_SPEECH_TO_TEXT);

// Find a matching etag:
$etag = $file->getEtag();
$assistantTask = null;
foreach ($tasks as $task) {
$taskEtag = $task->getInputsAsArray()['eTag'];
if ($taskEtag === $etag) {
$assistantTask = $task;
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why would there be multiple tasks with the same ocp_task_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an issue specific to the speech-to-text API; it doesn't have task ids so the only way to resolve the notification event back to the assistant task is by using the file id which is substituted for the ocpTaskId in this case. Then it's possible that you could have multiple tasks for the same file id (and possibly multiple different file versions)..?

@MB-Finski @julien-nc
Fix typos in app constants
421c140 
Co-authored-by: Julien Veyssier <[email protected]>
Signed-off-by: Sami Finnilä <[email protected]>
julien-nc
julien-nc requested changes Jan 31, 2024
View reviewed changes
lib/Service/AssistantService.php
Comment on lines +171 to +172
} catch (NotFoundException $e) {
// Ocp task not found, so we can't update the status
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should even return null if the OCP task can't be found. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's conceivable that an assistant meta task is stored for longer than the corresponding ocp task. This would be the case, for example, with generated images, if we unify the routes and front-end for all task categories at some point in the future. That's why I initially considered the generic catching of all exceptions acceptable here, since the primary task of the method has already been completed (although it definitely makes sense to do some debug logging).

lib/Service/AssistantService.php Outdated
$task = new TextProcessingTask($type, $input, $appId, $userId, $identifier);
$this->textProcessingManager->runTask($task);
return $task;
public function runTextProcessingTask(string $type, array $inputs, string $appId, ?string $userId, string $identifier): Task {
Copy link
Member

Choose a reason for hiding this comment

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

For later: We should avoid this trick to use "copywriter" as the task type because it's a category.
We could have 2 params to the run/schedule/run-or-schedule methods: category and textProcessingType (used for text processing only).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's true! I always thought of copywriter as just another text processing task but it might as well be considered another task category...

lib/Service/AssistantService.php Outdated Show resolved Hide resolved
lib/Service/FreePrompt/FreePromptService.php Outdated Show resolved Hide resolved
@julien-nc
fix passing task category prop to plain text modal
bfb7cf2 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
split STT result in 2 components: modal and result itself so we can d…
192edc2 
…isplay the results without the modal in the dedicated result page

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
rename task to metaTask everywhere for clarity, create a new table, d…
11a9537 
…rop the old one

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
rename Copywriter to 'Context write'
c5c94ae 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix resolveMetaTaskToOcpTask, keep OCA.TPAssistant.openAssistantResul…
15b317c 
…t for retro compatibility

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix resolving meta task to ocp task
7d446d0 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
use initial prompt as source material if copywriter gets selected
2591760 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc self-requested a review February 1, 2024 12:05
@julien-nc julien-nc merged commit 0e58541 into main Feb 1, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the add/copywriter branch February 1, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelklehr marcelklehr marcelklehr left review comments

@julien-nc julien-nc julien-nc approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@MB-Finski @julien-nc @marcelklehr