Entity creation #128

jimkang · 2022-09-07T20:15:48Z

This is an initial PR for the purposes of discussing the changes so far to support the creation of freestanding entities that aren't necessarily associated with documents.

Additions:

route that maps the /api/freestanding_entities endpoint to FreestandingEntityViewSet.
FreestandingEntityViewSet to handle GET and POSTs to the endpoint.
Unit test of existing _get_or_create_entities method.
Test for FreestandingEntityViewSet. Currently, this only runs when only the documentcloud/documents tests are run. When all of the tests are run with inv test this test fails.
EntityAdmin so that freestanding entities can be viewed in the Django admin UI.

mitchelljkotler · 2022-09-08T17:57:34Z

README.md

@@ -62,8 +62,12 @@ You must first have these set up and ready to go:
      - If you get an error on your console about signatures, fix minio as above.
      - If you get an error on your console about tipofday not found, add the static page as above.
 15. Develop DocumentCloud and its frontend!
+16. You can run the tests with `inv test`.
+- If you want to run a subset of the tests, you can specify the directory containing the test you want with the `path` switch like so: `inv test --path documentcloud/documents`.
+    - You can specify a single file in `--path` if you only want to run the tests in that file.


Nice addition to the docs. Maybe add how to run a single class or method?

mitchelljkotler · 2022-09-08T17:58:31Z

config/urls.py

@@ -67,6 +69,9 @@ class BulkNestedDefaultRouter(BulkRouterMixin, NestedDefaultRouter):
 documents_router.register(
    "modifications", ModificationViewSet, basename="modifications"
 )
+router.register(


I dislike the freestanding name - I think it makes sense to just label everything "Entity". Since it is not nested under documents there is no clash, and although they can be freestanding, the idea is always to link them to a document - there is no point to them otherwise.

That sounds good. I thought it might confuse existing users of the API because having the same entity either under documents or at the root level is a subtle distinction, but you know the users way better than I do.

mitchelljkotler · 2022-09-08T17:59:13Z

documentcloud/documents/admin.py

+class EntityAdmin(admin.ModelAdmin):
+    """Entity Admin"""
+
+    list_display = ("name", "kind", "mid", "description", "wikipedia_url")


You may want to read up on the admin options. I think description might be too much for the list display.

mitchelljkotler · 2022-09-08T17:59:41Z

documentcloud/documents/admin.py

+    """Entity Admin"""
+
+    list_display = ("name", "kind", "mid", "description", "wikipedia_url")
+    list_filter = ("kind", "description")


list_filter makes sense for kind (allow one to filter by kind) but not for description

mitchelljkotler · 2022-09-08T18:00:23Z

documentcloud/documents/admin.py

+
+    list_display = ("name", "kind", "mid", "description", "wikipedia_url")
+    list_filter = ("kind", "description")
+    search_fields = ("name", "kind", "mid", "description", "wikipedia_url")


This is a text search = I think name, mid and description make sense here (kind can be filtered by the list_filter options above, and searching through the wikipedia url does not make sense)

If you have the wikipedia url and want to know if there's an entity for it, how else would you find it? I'll drop kind.

mitchelljkotler · 2022-09-08T18:02:17Z

documentcloud/documents/admin.py

+            )
+        )
+
+    def delete_model(self, request, obj):


Documents require special clean up on deletion which the document destroy method handles. Entity's have no such requirement and do not have a destroy method. Leaving delete_model undefined so it uses the correct parent implementation.

mitchelljkotler · 2022-09-08T18:02:52Z

documentcloud/documents/rules/entities.py

 add_perm("documents.view_entityoccurrence", always_allow)
 add_perm("documents.add_entityoccurrence", is_authenticated)
 add_perm("documents.change_entityoccurrence", always_deny)
 add_perm("documents.delete_entityoccurrence", is_authenticated)
+add_perm("documents.view_entity", always_allow)
+add_perm("documents.add_entity", is_authenticated)


This should be fine for now, but I do think we need to think more about how the permissions should work.

mitchelljkotler · 2022-09-08T18:03:11Z

documentcloud/documents/tests/test_entity_extraction.py

+    "type_": 1,
+    "metadata": {"wikipedia_url": "https://en.wikipedia.org/wiki/Knight"},
+    "salience": 0.14932491,
+    "mentions": [


Maybe less mentions are necessary for testing purposes?

Ah, yes, that would be much more readable.

mitchelljkotler · 2022-09-08T18:05:11Z

documentcloud/documents/views.py

+        # TODO: Should everyone be able to view all entities?
+        return Entity.objects.all()
+
+    def perform_create(self, serializer):


I think we need to think through how entity creation works in more detail, but I am leaning away from having a POST do a get or create, and more toward multiple API calls. This can be abstracted in the Python SDK if necessary.

I am going to start a notion page to start a broader discussion about if entities should be all global as they are now, or if they should be per user or per organization and what the pros and cons are, and what analysis types they allow for and what we want to use entities for in the future.

mitchelljkotler · 2022-09-08T18:05:35Z

documentcloud/documents/views.py

+
+    def perform_create(self, serializer):
+        # pdb.set_trace()
+        """Initiate asyncrhonous creation of entities"""


Nothing asynchronous here, you are not calling it through celery

mitchelljkotler

Looks good, I think while we wait to see who the users for this are and what features we want to support for them, you could start adding on to the EntityOccurenceViewSet, to allow people to add manually create, update and delete entity occurrences to their documents. This might be a little tricky since I have the two custom methods to create occurrences using the Google API and to bulk delete all occurrences, but should be possible without changing those API calls. If you run into issues around that, let me know.

mitchelljkotler · 2022-09-09T13:47:00Z

documentcloud/documents/admin.py

+    """Entity Admin"""
+
+    list_display = ("name", "kind", "mid", "wikipedia_url")
+    list_filter = ("kind")


This needs to be a list or tuple.... one of python's downfalls is that ("string") == "string" and you need to do ("string",) with the trailing comma for a tuple of size 1

Whoa, very good to know.

mitchelljkotler · 2022-09-09T13:47:38Z

documentcloud/documents/admin.py

+    readonly_fields = ()
+
+    @transaction.atomic
+    def save_model(self, request, obj, form, change):


This method can be removed for now

mitchelljkotler · 2022-09-09T13:49:43Z

documentcloud/documents/views.py

@@ -911,3 +914,18 @@ def post_process(self, request, document_pk=None):
        post_process.delay(document_pk, request.data)

        return Response("OK", status=status.HTTP_200_OK)
+
+
+class FreestandingEntityViewSet(viewsets.ModelViewSet):


I would rename this to EntityViewSet and rename the current EntityViewSet to EntityOccurenceViewSet as that would be more accurate.

jimkang · 2022-09-09T19:14:35Z

@mitchelljkotler Just added the latest requested changes.

…though more testing is needed.

…k works.

…ant to use for now.

For future reference, you can break the string up into pieces to avoid the long line.

jimkang requested a review from mitchelljkotler September 7, 2022 20:15

mitchelljkotler reviewed Sep 8, 2022

View reviewed changes

mitchelljkotler reviewed Sep 9, 2022

View reviewed changes

jimkang and others added 26 commits December 12, 2022 11:02

Add Entity to Django admin page.

de50d6a

Add test commands to README.

c6b203d

Stub in entity extraction test. Function runs without exceptions so far.

22338e8

Add bulk_create option to _get_or_create_entities needed for test.

6af7420

Fix checking of items passed to bulk_create.

00ea60d

Fix test with wikipedia url.

251731a

cleaned up mocker usage

25e6fa1

WIP: test debugging.

edbf29f

Add test for (as yet unimplemented) freestanding entity endpoint.

d03d1d1

Start FreestandingEntityViewSet.

de11bd3

Fixed routing, rules, test data, and status code check. Test passes, …

2599df3

…though more testing is needed.

Check response content.

0da0eac

Restore accidentally removed permissions to fix pre-existing tests.

8227eb3

Move freestanding entity API tests into their own class.

e338b1d

Restore accidentally removed class.

e5529ee

Move freestanding entities out of the documents tests so that the moc…

8e18d61

…k works.

Add freestanding entities tests.

3284c93

Move freestanding entities to documents/.

2630cf4

Format.

0cedbfe

Get rid of unused imports.

44b0c09

Drop unnecessary import.

24eb328

Add instruction about running specific files and methods.

b3415d4

Rename endpoint from /api/freestanding_entities to /api/entities.

3897b02

Get rid of unnecessary code in entity admin.

3299a7b

Reduce number of mentions in test data.

e98c433

Get rid of untruthy comment.

42aa632

jimkang and others added 4 commits December 12, 2022 11:04

Fix string that should have been a tuple. Drop method that we don't w…

5554557

…ant to use for now.

Rename entity-related view sets.

9cc6782

patch the function where it is used

bbe541a

cleaner to compare to json

85d9344

For future reference, you can break the string up into pieces to avoid the long line.

jimkang force-pushed the entity-creation branch from 3a61d99 to 85d9344 Compare December 12, 2022 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity creation #128

Entity creation #128

jimkang commented Sep 7, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

jimkang Sep 8, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

jimkang Sep 9, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

jimkang Sep 9, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler Sep 8, 2022

mitchelljkotler left a comment

mitchelljkotler Sep 9, 2022

jimkang Sep 9, 2022

mitchelljkotler Sep 9, 2022

mitchelljkotler Sep 9, 2022

jimkang commented Sep 9, 2022

Entity creation #128

Are you sure you want to change the base?

Entity creation #128

Conversation

jimkang commented Sep 7, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchelljkotler left a 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

jimkang commented Sep 9, 2022