Api reverse links #139

liviuba · 2024-08-06T13:29:10Z

For https://app.clickup.com/t/86959dg0w , https://app.clickup.com/t/869527km2

Adding ObjectReference as it as (with the runtime validation hooks) just so we don't lose it when we squash, in case we ever have a usecase for it. Will make a PR deleting that part (so ObjectReference will just be a container for the link target) after that

Not fully tested, but tests should cover "one of each" for the major usecases (validating that a dependency object exists, and generating reverse links)

…we need it

…ocol

…t used anyway) Clarify pydantic.raises block, because I didn't know what was failing

liviuba · 2024-08-19T13:32:16Z

Note: Had to delete the ObjectReference type-validation-hooks because it broke schema chaining. Something to do with __get_pydantic_core_schema__, probably fixable but we were going to delete it in the next PR anyway.

Also, changes to the models tests are just to get a better exception for easier debugging (IMO - default error was just "Exception type not matching" and it took a lot of pytest flag fidgeting to get any meaningful info). Happy to undo

sherwoodf · 2024-08-19T14:14:48Z

bia-shared-datamodels/test/test_shared_models.py

-            ):
-                less_than_minimal_dict[key] = []
-                with pytest.raises(ValidationError):
+            if isinstance(less_than_minimal_dict[key], list):


I'm fine with the try-catch. Some super nit-picky comments:

the else on line 114 executes exactly the same code as in the else on line 130, so you could just check both conditions on line 96 & 97 and the do this logic.

I guess this also no longer checks a case where if a field is provided it must have 1 value, but can be not provided - which would be weird & require custom validation logic.

I think your code is clear enough to not need the comments you've included. If there is one that should have an explanation of what you're testing, its the comment one on line 99 since minimum cardinality requirements are unusual.

Makes sense, I think I defaulted to refining things by context, changed now

Was that checked previously? I didn't read it like that. But happy to tweak this in this PR if it was

Rephrased some of the comments, added to help keep track of branch context but happy to remove / trim down if you want the code to be neater

I think del less_than_minimal_dict[key] ... test run in all test cases, the

isinstance(less_than_minimal_dict[key], list) and len(less_than_minimal_dict[key]) > 0 ): ...``` conditions added an additional test for minimal list length: at least i presume this is the test logic if the test doesn't 'exit' on `with pytest.raises(...)` ?

sherwoodf · 2024-08-19T15:15:41Z

api/api/app.py

@@ -1,6 +1,7 @@
 from . import public
 from . import private
 from .models.repository import repository_create, Repository
+from bia_shared_datamodels.bia_data_model import ObjectReference, Study


Nit-pick: unused imports

sherwoodf · 2024-08-19T15:21:38Z

api/api/models/repository.py

+        ) in object_to_check.get_object_reference_fields().items():
+            if not getattr(object_to_check, link_attribute_name):
+                """
+                If a field optional and a link to another object,


Feels like something from this comment was deleted & it's a comment about the whole assert_model_doc_dependencies_exists logic, rather than specifically the logic in from like 93-101

It was actually meant to only cover that branch, just because it looks odd / handles the edge case of Optional[List[X]]. Changed the comment a bit, does that describe the context better?

sherwoodf · 2024-08-19T15:26:11Z

api/api/models/repository.py

+
+        docs = await self.biaint.find(
+            {"$or": doc_dependencies}, {"uuid": 1, "model": 1}
+        ).to_list(length=None)


nit-pick - explicitly saying length=None made me double check what this was doing

Agree it's annoying, but it's a required param. It probably should be some number instead of None for safety, will most likely change when I do pagination

sherwoodf · 2024-08-19T15:31:03Z

api/api/models/repository.py

+        if len(docs) != len(doc_dependencies):
+            """
+            ! This also fails if an object has the same object as a dependency twice.
+            Feature? Could it happen for something like links in different contexts to the same file/fileref?


Nothing currently has 2 links objects of the same class (except through generated endpoint links). It could be possible in the furture, but i would expect some of the fields to be 'calculateable'. I presume that since this is checking what we're putting in mongo, it's not testing generated fields.

I don't think I understand this. The main actual place where this would actually be useful (there's a test for it too, to make sure it stays consistent) is if we point to the same object multiple times in a list (which we technically treat as sets). This was an issue a while ago because diffing ingested studies that were changed is tricky.

This wasn't a feedback comment, was just reacting to the 'Could it happen...'

I think the accidentally pointing twice to the same object in the same field is probably a feature - most data we care about is like to be a set of unique values and rarely some intrinsic order (maybe some order for e.g. authors).

But when you said 'different context' i thought you mean different fields pointing to the same uuid. i was wondering if we could have a case where (and actually im realising as i'm typing this that i bet we do have this use-case and that the current models can't cope with it...) an Experimental Imaging Dataset with an annotation csv that is annotation if the images in that dataset. The csv, if it were an AnnotationFileReference, has both "submitted_in_dataset": <dataset123> and "source_dataset": <dataset123>.

sherwoodf · 2024-08-19T15:56:59Z

api/api/public.py

+            #   just raise and defer definining behaviour until we use it
+            if link_attribute_type in models_public:
+                router.add_api_route(
+                    f"/{to_snake(link_attribute_type.__name__)}/{{uuid}}/{to_snake(model.__name__)}",


🚩 You're using the type of the object to create the field names for the reverse links, right?

That feels not ideal when the field names of the forwards links aren't the names of the classes, but it does follow the type pattern of api definitions. Eurgh, i'm not sure what would be better here. But this method no longer works if we ever have 2 links in the same direction between two classes (which we don't currently, in the diagram, and we don't have plans for, but could be possible in the future).

Will we eventually generate the forward links as paths as well? My understanding was this was the eventual intent - if so we start getting weird discrepancies between the uuid field names and the path/endpoint.

For the paths, I hadn't thought of that. Makes sense that it could come up as something we might need. The thing is we only have the "arrow origin" object in that context. Could add some extra params to the annotation to control what the reverse link shows up as, when we need it?

Also for the forward links, I didn't have those in mind here, the forward "links" would basically be get the dataset, then get the study (vs this which is "get datasets for some study"). The reason we needed the reverse links was so that we could actually find things that point to the thing upstream, so we could walk down the model. Like for example in biaint, if I had a study and wanted to know which datasets does it have, wouldn't have a way to do that without them. It would be a nice convenience feature though, but the thing we would actually need to be able to walk through everything that's in a study was this AFAICT.

Yep, totally, this was definitely the higher priority feature to get implemented. I was just bringing up the forward links here because the naming of them might influence our decision here.

I think extra params to the annotation might be the nicest option if you're okay with them not being the class names?

Yep, totally, do you think it should go in here, or a ticket to do later once we release? I think it would be a default-first sort of thing (with the default being the class name) just to avoid making it too verbose. We would also need to come up with a way to add that information without calling it api_reverse_link_fragment (or similar) to make it usecase-independent. Something like association_id I guess?

do you think it should go in here, or a ticket to do later once we release

I'm happy to approve this PR without the feature in the name of iterative development, smaller PRs being faster to merge etc. Depends what you mean by release - i think it's okay to get the website working with this since it's only going to mean that i'll need to make changes to the export code once the client updates, but i think it's a high priority ticket before anyone else starts using the client, since it would be quite a big change to a lot of endpoints.

yep, agree, added https://app.clickup.com/t/8695fkkmj

sherwoodf

LGTM

liviuba temporarily deployed to test August 6, 2024 13:29 — with GitHub Actions Inactive

liviuba temporarily deployed to test August 6, 2024 15:41 — with GitHub Actions Inactive

liviuba force-pushed the api_reverse_links branch from a7bf304 to 48efe5e Compare August 7, 2024 15:03

liviuba had a problem deploying to test August 7, 2024 15:03 — with GitHub Actions Failure

liviuba had a problem deploying to test August 8, 2024 12:49 — with GitHub Actions Failure

liviuba had a problem deploying to test August 8, 2024 15:41 — with GitHub Actions Failure

liviuba had a problem deploying to test August 14, 2024 10:51 — with GitHub Actions Failure

liviuba had a problem deploying to test August 14, 2024 13:51 — with GitHub Actions Failure

liviuba had a problem deploying to test August 14, 2024 15:22 — with GitHub Actions Failure

liviuba had a problem deploying to test August 14, 2024 15:39 — with GitHub Actions Failure

liviuba added 11 commits August 19, 2024 13:22

Add reverse links

c6d5b39

Typing fixes

c5407a6

More type fixes

89c3be3

Add example model referencing list of objects

714c7b2

wip Add generated reverse links

8664f5a

Fix Annotated[] types not chained causing not to be converted to UUID

ad3cf10

wip add pluginnable validator test (deleteme)

6172054

Remove uses of ObjectReference for validation hooks. Left in in case …

af5ea69

…we need it

Refactor "reverse links" in public, add doc dependency validation

b812531

Add Annotated/branching todo everywhere

a0ac7dd

Add dependency check / reverse link test for Optional[List[UUID]]

20378c5

liviuba added 2 commits August 19, 2024 13:22

typos

eb936c0

Formatting

85b74dd

liviuba force-pushed the api_reverse_links branch from a154965 to 85b74dd Compare August 19, 2024 12:22

liviuba had a problem deploying to test August 19, 2024 12:22 — with GitHub Actions Failure

Delete placeholder failing tests

9b8cc49

liviuba had a problem deploying to test August 19, 2024 12:33 — with GitHub Actions Failure

SpecimenImagingPrepartionProtocol -> SpecimenImagingPrepar*a*tionProt…

340ba9e

…ocol

liviuba had a problem deploying to test August 19, 2024 12:37 — with GitHub Actions Failure

liviuba added 2 commits August 19, 2024 13:40

Delete unused dependency

87a6f50

Drop type validation hooks because they broke chained validators (&no…

33f634c

…t used anyway) Clarify pydantic.raises block, because I didn't know what was failing

liviuba had a problem deploying to test August 19, 2024 13:28 — with GitHub Actions Failure

liviuba requested a review from sherwoodf August 19, 2024 13:33

liviuba marked this pull request as ready for review August 19, 2024 13:34

sherwoodf mentioned this pull request Aug 19, 2024

Multi study ingest & export #152

Merged

sherwoodf reviewed Aug 19, 2024

View reviewed changes

Add review suggestions

7fc8bd0

liviuba had a problem deploying to test August 20, 2024 09:18 — with GitHub Actions Failure

liviuba requested a review from sherwoodf August 20, 2024 09:18

Remove else

2a66d58

liviuba had a problem deploying to test August 20, 2024 09:29 — with GitHub Actions Failure

sherwoodf approved these changes Aug 20, 2024

View reviewed changes

sherwoodf merged commit 897014a into main Aug 20, 2024
30 of 33 checks passed

sherwoodf deleted the api_reverse_links branch August 20, 2024 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api reverse links #139

Api reverse links #139

liviuba commented Aug 6, 2024 •

edited

Loading

liviuba commented Aug 19, 2024

sherwoodf Aug 19, 2024

liviuba Aug 20, 2024 •

edited

Loading

sherwoodf Aug 20, 2024 •

edited

Loading

sherwoodf Aug 19, 2024

sherwoodf Aug 19, 2024

liviuba Aug 20, 2024

sherwoodf Aug 19, 2024

liviuba Aug 20, 2024

sherwoodf Aug 19, 2024

liviuba Aug 19, 2024

sherwoodf Aug 20, 2024

sherwoodf Aug 19, 2024

liviuba Aug 19, 2024 •

edited

Loading

sherwoodf Aug 20, 2024 •

edited

Loading

liviuba Aug 20, 2024

sherwoodf Aug 20, 2024

liviuba Aug 20, 2024

sherwoodf left a comment

Api reverse links #139

Api reverse links #139

Conversation

liviuba commented Aug 6, 2024 • edited Loading

liviuba commented Aug 19, 2024

Choose a reason for hiding this comment

liviuba Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

sherwoodf Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liviuba Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

sherwoodf Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherwoodf left a comment

Choose a reason for hiding this comment

liviuba commented Aug 6, 2024 •

edited

Loading

liviuba Aug 20, 2024 •

edited

Loading

sherwoodf Aug 20, 2024 •

edited

Loading

liviuba Aug 19, 2024 •

edited

Loading

sherwoodf Aug 20, 2024 •

edited

Loading