Closed juhoinkinen closed 1 year ago
Patch coverage: 100.00
% and no project coverage change.
Comparison is base (
38ec228
) 99.57% compared to head (ca158d8
) 99.58%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Would it make sense to limit the number of documents in a single REST call to the minibatch size (currently 32)? After all, there would be little benefit (except maybe avoiding some HTTP overhead) in processing more than one minibatch on the backend side.
I tried to reduce duplication in annif.yaml
as much as possible. Unfortunately I did not find a way to define parameters for suggestions in only place, because for /suggest
they are used in request body, whereas for /suggest-batch
in query parameters.
Edit: Defining the suggest parameters in components.parameters
works otherwise, but then the types of the variables or their default values do not show in the OpenAPI/Swagger online documentation.
I reverted two refactoring commits. I think https://github.com/NatLibFi/Annif/commit/067fec38223adb1c1e957fe480ad9e6878e5147c should not have been problematic, but I run openapi-diff tool, and it complained about POST /projects/{project_id}/suggest
:
==========================================================================
== API CHANGE LOG ==
==========================================================================
Annif REST API
--------------------------------------------------------------------------
-- What's New --
--------------------------------------------------------------------------
- POST /projects/{project_id}/suggest-batch
--------------------------------------------------------------------------
-- What's Changed --
--------------------------------------------------------------------------
- POST /projects/{project_id}/learn
Request:
- Changed application/json
Schema: Backward compatible
- POST /projects/{project_id}/suggest
Request:
- Changed application/x-www-form-urlencoded
Schema: Broken compatibility
Changed property type: (object -> object)
--------------------------------------------------------------------------
-- Result --
--------------------------------------------------------------------------
API changes broke backward compatibility
--------------------------------------------------------------------------
Now after the reverts the tool still sees changed application/json in /projects/{project_id}/learn
, but I think it is wrong, at least I cannot see a difference in there, and the tool anyway says that the change is backward compatible.
Edit: Okey, openapi-diff
probably noticed the change in the description of IndexedDocument schema.
/suggest-batch
, but no test for it.Schemathesis uses the examples from OpenAPI specification and some random inputs in path and query parameters. The requests to /v1/projects/<proj-id>/suggest-batch
can be seen by running a local Annif server and Schemathesis from command line:
st run annif/openapi/annif.yaml -E suggest-batch$ --base-url http://127.0.0.1:5000/v1
But there is no request with 400 code, which is the code for language_not_supported_error
or too many documents.
Finally I managed to drop the unnecessary merge and revert commits. I recreated the PR branch from the current main and then cherry-picked the good commits from a backup PR branch. I don't know why rebasing did not work: there ended up also all commits made to main not matter what I tried. Changing base branch back and forth did not help as usually.
But now there is a problem installing just released version of a dependency in GH Actions (but not on my laptop):
Unable to find installation candidates for libclang (16.0.0)
But now there is a problem installing just released version of a dependency in GH Actions (but not on my laptop):
Unable to find installation candidates for libclang (16.0.0)
This seemed to be caused by issue https://github.com/sighingnow/libclang/issues/46, which got fixed.
- I suggested a little change to the wording of the method summary
Done.
- I verified that the method fails if given more than 32 documents, as it should. But the error message is a bit confusing:
{ "detail": "[{'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'Olipa kerran'}, {'text': 'plaa'}] is too long - 'documents'", "status": 400, "title": "Bad Request", "type": "about:blank" }
Basically the "detail" field includes the whole request, which could be extremely long; and it ends with "is too long - 'documents'" which is not that helpful. Is this something we could change or is this coming directly from Connexion so we can't do anything about it? I would like to see a more helpful message, which wouldn't include the whole request body, just a message stating that there were too many documents.
I added CustomRequestBodyValidator
in annif/openapi/validation.py
module. It is a child class of the connexion RequestBodyValidator
, and it overrides the default validate_schema()
method to modify the message in the "detail" field to only validation error: too many items - 'documents'
.
This seems to be the recommended way to modify the validation, found it via https://github.com/spec-first/connexion/issues/558.
- Related to the above, can we write (for example using schemathesis) unit tests that verify that the API accepts 32 documents, but doesn't accept 33 documents? I'm worried that if we change the implementation later, the limit of 32 will be dropped and it could lead to problems on the backend side.
I added tests for cases 32 and 33 documents, and restored the manually written Swagger/OpenAPI tests that I removed in #682. These test do not rely on Schemathesis but on the app_client
fixture, and this allows to check which error code is given for which error.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell
No Coverage information
0.0% Duplication
Adds a new
/v1/projects/{project_id}/suggest-batch
REST API method. Based on the branch of PR #663, implements the REST API part of #579.The method accepts in json the documents (max. 32) with the optional
document_id
field:The
limit
,threshold
andlanguage
parameters are optional as for the regularsuggest
method and can be given as URL query parameters:An example response is:
The
document_id
is null in the response if the document in the request does not have one. It is similar to theexternal_id
field of MonkeyLearn classifier and to theindex
AWS Comprehend BatchDetectKeyPhrases.