fix: allow doclist to have nested optional document by samsja · Pull Request #1472 · docarray/docarray

samsja · 2023-04-27T15:44:35Z

Context

This pr fix #1468

What this PR do

This pr do two things related to Optional field

DocList support for Optional field

from docarray.typing import NdArray
import numpy as np

class ImageDoc(BaseDoc):
    tensor: NdArray

class ArticleDoc(BaseDoc):
    image: Optional[ImageDoc]
    title: str

docs = DocList[ArticleDoc](
    [
        ArticleDoc(image=ImageDoc(tensor=np.ones((3, 224, 224))), title="Hello"),
        ArticleDoc(image=None, title="World"),
    ]
)

Before calling docs.image would raise an error because it will try to return a DoctList[ImageDoc] but the None value cannot he pass into such a DocList.

This PR change this behavior (this is a breaking change) and now return a list of the collected object. It can contain None or a ImageDoc.

DocVec support for Optional field

from docarray.typing import NdArray
import numpy as np

class ImageDoc(BaseDoc):
    tensor: NdArray

class ArticleDoc(BaseDoc):
    image: Optional[ImageDoc]
    title: str

docs = DocVec[ArticleDoc]([ArticleDoc(title="Hello") for _ in range(10)])

before you could not construct such a DocVec because you the doc are set to None.

We enforce as well the following behavior: if one field is None all of the other field must be None. An error will be raise at init time if this is not the case.

This pr allows this. Instead of storing all of the None it will just store None for this column.

This behavior is uniformized for the tensor field as well.

This is a breaking change. Before we were casting the None value into Nan value of each of the tensor backends and we would store the whole column of Nan value.

Now this is different we just store the column as None.

Example usage

    class Features(BaseDoc):
        tensor: NdArray[100]

    class Image(BaseDoc):
        url: ImageUrl
        features: Optional[Features]

    docs = DocVec[Image](
        [Image(url='http://url.com/foo.png') for _ in range(10)]
    )

    print(docs.features) # None

    docs.features = [Features(tensor=np.random.random([100])) for _ in range(10)]
    print(docs.features) # <DocVec[Features] (length=10)>
    assert isinstance(docs.features, DocVec[Features])

    docs.features.tensor = np.ones((10, 100))

    assert docs[0].features.tensor.shape == (100, )

    docs.features = None

    assert docs[0].features is None

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

JohannesMessner · 2023-05-03T15:41:09Z

This behavior is uniformized for the tensor field as well.

This is a breaking change. Before we were casting the None value into Nan value of each of the tensor backends and we would store the whole column of Nan value.

is this breaking change necessary? can't we just keep nan values in the respective backend format?

samsja · 2023-05-03T16:12:40Z

is this breaking change necessary? can't we just keep nan values in the respective backend format?

Hmm not it is none. But this is a way harder story to tell if we have different behavior for Optional[Tensor] or Optional[BaseDoc]. I would rather have the same behavior. Plus I think this one is way more optimized and solve benchmark problem we had before wit pytorch. Because we don't carry long none value anymore

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

…into fix-docarray-optional

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

…into fix-docarray-optional

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

JohannesMessner

Sorry to be pain in the butt, a lot of suggestions :D

But honestly it seems like the entire doc section could be condensed down to a few sentences:

Nested optionals are a bit tricky, watch out
For doclist the return will be a DocList instead of DocList[...]
For docvec the return will be DocList[...] if all documents are there, or None if all docs are None. No mix of docs and None allowed!

I would lead with these "rules", I think they are most important to the user. And after that we can go into an explanation of why that is, maybe even hidden away behind a collapsed box.

Otherwise I think it is difficult for the users to tease out what is the important information

JohannesMessner · 2023-05-04T11:59:39Z

+!!! note
+    This whole section is specific for nested BaseDoc, DocList and DocVec. 


If this is the case then the heading for this should be different, something like "dealing with nested, optional fields" or something like that

JohannesMessner · 2023-05-04T12:05:49Z

+assert docs.image == [ImageDoc(tensor=np.ones((3, 224, 224))), None]
+```
+
+but for DocVec it is a bit different. Indeed, DocVec store the data for each filed as contiguous column. 


What does the fact that it stores it as a column to do with it not being a DocList? That column could in theory be a DocList, right?

well not if some document are none ?

JohannesMessner · 2023-05-04T12:08:09Z

+
+
+```python
+from docarray import DocVec


maybe use tabs for these examples?

I split it a bit differently now

samsja · 2023-05-04T12:25:00Z

Sorry to be pain in the butt, a lot of suggestions :D

But honestly it seems like the entire doc section could be condensed down to a few sentences:
* Nested optionals are a bit tricky, watch out

* For doclist the return will be a `DocList` instead of `DocList[...]`

* For docvec the return will be `DocList[...]` if all documents are there, or `None` if all docs are None. No mix of docs and None allowed!
I would lead with these "rules", I think they are most important to the user. And after that we can go into an explanation of why that is, maybe even hidden away behind a collapsed box.

Otherwise I think it is difficult for the users to tease out what is the important information

sure I will follow your suggestion

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

…nto fix-docarray-optional

JohannesMessner · 2023-05-05T09:56:49Z

    or extend your data.

+
+## Dealing with Optional fields


Suggested change

## Dealing with Optional fields

## Dealing with nested Optional fields

Everything below only applies to the nested case, right?

JohannesMessner · 2023-05-05T10:01:42Z

+
+    For nested BaseDoc 
+    * DocList will return a list of document if the field is optional and a DocList if the field is not optional
+      * DocVec will return a DocList if all documents are there, or None if all docs are None. No mix of docs and None allowed!


Shouldn't DocVec return a DocVec? Am i remembering correctly that we wanted to do that but haven't implemented it yet?

shit you are right ofc

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

github-actions · 2023-05-05T12:59:17Z

📝 Docs are deployed on https://ft-fix-docarray-optional--jina-docs.netlify.app 🎉

he approve

fix: allow doclist to have nested optional document

1e4f0a7

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

github-actions Bot added size/s area/core area/testing component/array labels Apr 27, 2023

JoanFM linked an issue Apr 27, 2023 that may be closed by this pull request

Creating DocVec with the Object that has Optional Field #1468

Closed

Joan Fontanals and others added 2 commits April 28, 2023 11:46

Merge branch 'main' into fix-docarray-optional

54aa32f

refactor: rename test

35b41e0

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

samsja marked this pull request as ready for review May 2, 2023 12:14

samsja marked this pull request as draft May 2, 2023 12:15

refactor: allow doc vec column to be None

264bd84

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

github-actions Bot added size/m and removed size/s labels May 3, 2023

samsja added 5 commits May 3, 2023 15:05

refactor: add better error message when DocVec fail

910864f

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

fix: add more test on none mode

ba50e17

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

docs: update docstring for doc vec

28d40ed

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

docs: add docstring to doc list

593578e

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

docs: add docs about optional field

733bfe6

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

github-actions Bot added size/l area/docs and removed size/m labels May 3, 2023

docs: add warning

b6c59cf

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

samsja marked this pull request as ready for review May 3, 2023 15:08

feat: add integreation test

e07d1f6

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

JoanFM approved these changes May 3, 2023

View reviewed changes

JohannesMessner requested changes May 3, 2023

View reviewed changes

samsja and others added 2 commits May 4, 2023 09:09

fix: apply joahnnes docs suggestion

43ccfbf

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

refactor: use none directly

d482752

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

samsja and others added 8 commits May 4, 2023 09:13

Merge branch 'fix-docarray-optional' of github.com:docarray/docarray …

6ec5a4f

…into fix-docarray-optional

feat: apply johannes suggestion

07bb27b

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

fix: fix is none

7107ccb

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

feat: apply johannes suggestion

9947cd2

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

fix: fix last things

1a78450

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

Merge branch 'fix-docarray-optional' of github.com:docarray/docarray …

123661b

…into fix-docarray-optional

fix: ad missing check for nested optional doclist

49a142d

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

fix: fix documentation docs

8ecc117

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

JohannesMessner previously requested changes May 4, 2023

View reviewed changes

samsja and others added 4 commits May 4, 2023 15:05

feat: apply johannes suggestion

4c8eb1a

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

fix: fix docs

6ef95b6

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

fix: fix docs

dcf07d2

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

Merge branch 'fix-docarrayoptional' of github.com:docarray/docarray i…

f60c1e3

…nto fix-docarray-optional

JoanFM requested a review from JohannesMessner May 5, 2023 09:49

JohannesMessner reviewed May 5, 2023

View reviewed changes

samsja and others added 3 commits May 5, 2023 14:45

feat: apply johannes suggestion

cea57eb

Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com> Signed-off-by: samsja <55492238+samsja@users.noreply.github.com>

docs: fix mistake

38c80cc

Signed-off-by: samsja <sami.jaghouar@hotmail.fr>

Merge branch 'main' into fix-docarray-optional

cc00c9d

samsja merged commit b77df16 into main May 5, 2023

samsja deleted the fix-docarray-optional branch May 5, 2023 13:30

samsja mentioned this pull request May 8, 2023

v0.31.0 release note draft #1456

Closed

		!!! note
		This whole section is specific for nested BaseDoc, DocList and DocVec.

	## Dealing with Optional fields
	## Dealing with nested Optional fields

Conversation

samsja commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

What this PR do

DocList support for Optional field

DocVec support for Optional field

Example usage

Uh oh!

JohannesMessner commented May 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samsja commented May 3, 2023

Uh oh!

JohannesMessner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsja commented May 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

samsja commented Apr 27, 2023 •

edited

Loading