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

David/use dependency injection #168

Conversation

recalcitrantsupplant
Copy link
Collaborator

@recalcitrantsupplant recalcitrantsupplant commented Oct 27, 2023

See last commit only, others are from other open PR.

  • Abstracts queries to use Repos/FastAPI's Dependency injection. Three kinds have initially been implemented; remote SPARQL endpoints using httpx, local Pyoxigraph stores, and local RDFLib Oxrdflib stores. The Oxrdflib store has not been extensively tested.
  • Switches to using pyoxigraph for testing. Oxrdflib was attempted however adds (not incorrectly) xsd:string datatypes to literals, where Fuseki/Pyoxigraph do not, which means all of the reference responses would need to be updated.
  • Pyoxigraph appears to have issues with Count queries - a few queries in the VocPrez implementation have been xfailed due to this
    • in future I would recommend we run an in memory Fuseki instance for tests + automating this in the GH action, as it is most likely to adhere to SPARQL 1.1
  • Adds caching for prez:link and prez:identifier to assist with immediate performance problems where responses contain a large number of URIs.
  • @edmondchuc there's an issue w/ the SPARQL query in get_concept_scheme_top_concepts_query - it appears to be creating top concept triples where they don't exist if run on the second test case in test_concept_scheme_top_concepts. This is also a test that is failing (additionally) due to an incorrect count when Pyoxigraph runs the query, so it is xfailed at present.

Specify namespace to avoid race condition
Add print to identify why GH action failing but local passing
Add further ref data (SKOS + SDO HTTPS version)
update GH actions; add init.py for tests
add oxrdflib depedency
QuerySender -> Repo
Use Fastapi's dependency injection to allow switchable backends
Add caching specifically to link generation
@edmondchuc
Copy link
Collaborator

I think it'll be easier to review if we just target the merge into #164 instead of main.

Can we change the merge target into #164 instead of main? This already includes the content of the other PR, if we target that PR, we will get a clean diff here and be able to review it as normal.

@recalcitrantsupplant recalcitrantsupplant changed the base branch from main to david/annotations-endpoint-catprez-endpoints November 6, 2023 01:56
@recalcitrantsupplant
Copy link
Collaborator Author

@edmondchuc I've changed the target

@edmondchuc
Copy link
Collaborator

Perfect thanks, I'll take a look at it now!

Copy link
Collaborator

@edmondchuc edmondchuc left a comment

Choose a reason for hiding this comment

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

I think the PR is fine for now as it addresses the immediate issues with the way we run tests. Nice work! 🎉 I can see that the tests run around 4 times faster, which is a nice bonus.

A follow-up to improve the structure of the codebase for long-term maintenance can look something like this:

/
├── repositories/
│   ├── __init__.py
│   ├── remote_sparql.py
│   ├── pyoxigraph.py
│   └── oxrdflib.py
├── services/
│   ├── __init__.py
│   ├── object.py
│   ├── listing.py
│   └── etc...
└── routers/
    ├── __init__.py
    ├── object.py
    ├── listing.py
    ├── sparql.py
    ├── vocprez.py
    ├── catprez.py
    └── spaceprez.py
  • Repositories are the interfaces to the data store.
  • Services are the business logic for each of facets of functionality within Prez. They only contain business logic and interact with the data store through a repository object.
  • Routers are the API endpoints of Prez. They are the entrypoints to each facet of business logic of Prez and only deal with the HTTP layer. The service and repo objects are injected via FastAPI's DI.

I think by having these three layers at a minimum will allow us all to conceptually understand Prez and its implementation details a bit clearer.

Anyway, I think this can probably be followed up in a separate PR as it's more maintenance-related. If you agree with the above's proposed structure, I can create an issue and reference this PR's comment.

prez/config.py Show resolved Hide resolved
prez/config.py Outdated Show resolved Hide resolved
@recalcitrantsupplant
Copy link
Collaborator Author

@edmondchuc Thanks for the layout feedback - I've known it needs a refactor for some time but haven't looked at any reference implementations so that will be helpful, would be good to add to the wiki/documentation if it's online now

reformat with black
@edmondchuc
Copy link
Collaborator

@recalcitrantsupplant - great! Good idea with adding the design patterns into the wiki.

@recalcitrantsupplant recalcitrantsupplant merged commit bde711b into david/annotations-endpoint-catprez-endpoints Nov 8, 2023
1 check passed
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.

2 participants