Skip to content

Conversation

@lukasdotcom
Copy link

Currently the types for optional_input_shape and probably also optional_output_shape are different on the nextcloud server and how they are defined in this package.

Before:

optional_input_shape=[
     ShapeDescriptor(
         name="voice",
         description="Voice to use",
         shape_type=ShapeType.ENUM)]

After:

optional_input_shape={
 "voice":
     ShapeDescriptor(
         name="voice",
         description="Voice to use",
         shape_type=ShapeType.ENUM)}

@codecov
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.52%. Comparing base (0965823) to head (7776a4e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   94.52%   94.52%           
=======================================
  Files          46       46           
  Lines        5389     5389           
=======================================
  Hits         5094     5094           
  Misses        295      295           
Files with missing lines Coverage Δ
nc_py_api/ex_app/providers/task_processing.py 95.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lukasdotcom
Copy link
Author

Adding to this that the server side validation might also be wrong, it actually wants:

dict[str, list[ShapeDescriptor]]

If you look into the nextcloud server though the type definition for getOptionalInputShape is what I based the type on:

ShapeDescriptor[]
[
	'images' => new ShapeDescriptor(
		$this->l->t('Output images'),
		$this->l->t('The generated images'),
		EShapeType::ListOfImages
	),
]

If it is dict[str, ShapeDescriptor] then app_api's validation logic will need to be modified.

@kyteinsky
Copy link
Contributor

hello,
thanks for the fix!
AppAPI's checks also need to be adjusted for this. Would you mind creating a PR for that too?
The relevant lines should be: https://github.com/nextcloud/app_api/blob/bf4085d8791a2bc20b6845688a7a6533d46006d9/lib/Service/ProvidersAI/TaskProcessingService.php#L119-L124

@lukasdotcom
Copy link
Author

Implemented fix in AppApi at nextcloud/app_api#548

@kyteinsky
Copy link
Contributor

Discussed with Marcel.
It would be better to keep the definitions consistent, use list for both input_shape (in TaskType) and optional_input_shape.
And we cannot change the type for input_shape in TaskType as it would break compatibility with previous versions.

So let's close this PR and fix the checks in the app_api repo for the optional_input_shape and optional_output_shape.
The working code for conversion of input_shape of the custom task type: https://github.com/nextcloud/app_api/blob/31577f00f58317165a9f7eea01537f541aae8970/lib/Service/ProvidersAI/TaskProcessingService.php#L361-L381
This same conversion should be done here: https://github.com/nextcloud/app_api/blob/31577f00f58317165a9f7eea01537f541aae8970/lib/Service/ProvidersAI/TaskProcessingService.php#L272-L290
and the validation needs to be updated.

What do you think about it?

@lukasdotcom
Copy link
Author

That sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants