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

Add reconciliation API endpoint to REST API #734

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

UnniKohonen
Copy link
Contributor

@UnniKohonen UnniKohonen commented Sep 8, 2023

This PR adds a Reconciliation Service API endpoint to REST API as mentioned in #338. It allows reconciling against a project using the /v1/projects/{project_id}/reconcile API method. It also includes a suggest service for entities with the /v1/projects/{project_id}/suggest/entity API method.

The main reconciliation endpoint accepts GET and POST requests:

Only the fields query and limit are supported in the queries objects. Reconciliation result objects contain the fields id, name, score and match. Reconciliation doesn't support types or properties for now.

The reconciliation API specification requires the usage of the identifierSpace field in the service manifest. Since Annif doesn't require entities to be in a specific URI space and doesn't have a way to extract this information from a project, the field is left empty for now.

The suggest service endpoint accepts GET requests and returns a suggest response. It supports the prefix and cursor parameters.

Closes #338

@osma
Copy link
Member

osma commented Sep 15, 2023

Excellent work!

The reconciliation API specification requires the usage of the identifierSpace field in the service manifest. Since Annif doesn't require entities to be in a specific URI space and doesn't have a way to extract this infomation from a project, the field is left empty for now.

I opened an issue about this problem: reconciliation-api/specs#139

I will soon provide more detailed comments about the code.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

See the detailed line-by-line comments.

In addition, tests/test_openapi.py is failing on GitHub Actions as well as for me locally. There seems to be some problem with the queries parameter. Possibly the OpenAPI spec needs to be adjusted somehow.

annif/rest.py Outdated
@@ -214,3 +215,72 @@ def learn(
return server_error(err)

return None, 204


def _reconcile(project_id: str, query: dict[str, Any]) -> dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

The return type hint dict doesn't seem to match the actual returned type, which is a list of dicts.

(found by SonarCloud)


queries = body["queries"]
results = {}
for key, query in queries.items():
Copy link
Member

Choose a reason for hiding this comment

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

Using a for loop here means that the reconciliation isn't done in batches internally. It would be better (more efficient) to pass the whole batch to _suggest at once, similar to how the suggest_batch method works.

@@ -314,6 +460,105 @@ components:
type: string
example: Vulpes vulpes
description: A document with attached, known good subjects
ReconcileMetadata:
Copy link
Member

Choose a reason for hiding this comment

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

It could be better to call this type ReconciliationServiceManifest (although it's pretty long...) so that it matches the reconciliation API spec.

assert "result" in results["q0"]


def test_rest_reconcile_nonexistent(app):

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning test

This assignment to 'test_rest_reconcile_nonexistent' is unnecessary as it is
redefined
before this value is used.
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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

Successfully merging this pull request may close these issues.

OpenRefine reconciliation API
2 participants