Add circuit hierarchy endpoint #320
Conversation
GianlucaFicarelli
changed the title
app/routers/hierarchy.py
Outdated
| tags=["hierarchy"], | ||
| ) | ||
|
|
||
| router.get("/circuit")(app.service.hierarchy.read_circuit_hierarchy) |
There was a problem hiding this comment.
Note: we can decide for a different endpoint name, I don't have a string opinion.
- /hierarchy/circuit: it can be useful if we think we can reuse the same logic for other entities
- /circuit-hierarchy: it's specific for circuits
- ...
There was a problem hiding this comment.
I don't have a strong preference, I usually like things organized like circuit/hierarchy, but that's just how my brain works.
There was a problem hiding this comment.
/circuit/hierarchy could work for me as well, however it should be added before /circuit/{uuid}, because FastAPI evaluates routes in the order they are added.
There was a problem hiding this comment.
To me as long as there is either a prefix or postfix for circuit I am fine. /circuit/hierarchy seems more intuitive but I don't have a too strong of an opinion. From the sdk side it doesn't really matter because there will be a dedicated method that will construct the path in either way.
It would be nice if we could eventually reuse the construction of the hierarchy for other entities that have derivations too and have some sort of a common query function as we have for read/create
There was a problem hiding this comment.
If we change the route from /hierarchy/circuit to /circuit/hierarchy I can move the endpoint definition in the to routers/circuit.py, but keep service/hierarchy.py, as the logic isn't very specific to the circuit.
It can remain generic as long as we don't add attributes that are specific to the circuit, or we'll need different schemas for the response.
|
LGTM, thanks @GianlucaFicarelli |
|
|
||
| class HierarchyTree(BaseModel): | ||
| derivation_type: DerivationType | ||
| data: list[HierarchyNode] |
There was a problem hiding this comment.
The latest changes take in account what have been discussed and mentioned in this comment:
#292 (comment)
Side notes:
derivation_typeis now moved at the top level of the response payload, because it's the same for every relation.- I added
authorized_publicandauthorized_project_idto the returned entities, because they should make it easier to debug issues with the expected response. We can decide now or later to make them optional to reduce the payload size. - I kept
parent_id: it's not strictly needed because it's implicit in the tree structure, but it's used when the tree is built so it's just easier to dump the full payload. We can decide now or later to remove it.
There was a problem hiding this comment.
Unless we remove things now, they will probably be required forever :)
OTOH, we haven't really been too careful about optimizing payloads thus far, so maybe we don't need to start now.
There was a problem hiding this comment.
Yeah, I would leave these fields (authorized_public, authorized_project_id and parent_id) unless we see that the payload is too big.
For reference, although it can change, the current ALL_CIRCUITS.json contains 155 circuits and it's around 7.5 MB but it has a lot more information (especially metadata like contributors, organizations, publications...)
There was a problem hiding this comment.
yup, makes sense
155 circuits and it's around 7.5 MB
wow; 40k per circuit? nuts
|
lgtm, things are more clear now |
* origin/main: IonChannelModel inheriting from ScientificArtifact (#300) Add external data source and url (#327) Add Calibration & Validation activities (#307) Update uv and relax uv pinning in Dockerfile (#336) Add filter by authorized_public and authorized_project_id (#335) Fix legacy attributes (#328) Add circuit hierarchy endpoint (#320) Remove contribution from publication (#331)
…-model * origin/main: Add the ability to filter by name to IonChannelModel (#338) IonChannelModel inheriting from ScientificArtifact (#300) Add external data source and url (#327) Add Calibration & Validation activities (#307) Update uv and relax uv pinning in Dockerfile (#336) Add filter by authorized_public and authorized_project_id (#335) Fix legacy attributes (#328) Add circuit hierarchy endpoint (#320) Remove contribution from publication (#331) Add timezone to scientific_artifact.experiment_date (#326)
Fix #292