feat: add multimodal document #1335

bwanglzu · 2020-11-23T22:12:31Z

Add MultimodalDocument to primitive types, and apply changes to MultimodalDriver.

Example usage of a MultimodalDocument?

# With a list of documents (chunks)
md = MultimodalDocument(chunks=[chunk1, chunk2])
md = MultimodalDocument.from_chunks(chunks=[chunk1, chunk2])
# With modality content mapping (dict representation of modality and document content)
mapping = {'visual': 'visual content', 'textual': 'textual content'}
md = MultimodalDocument(modality_content_mapping=mapping)
md = MultimodalDocument.from_modality_content_mapping(modality_content_mapping=mapping)
# Exposed the modality content mapping as a property
md.modality_content_mapping
>>> {'visual': 'visual content', 'textual': 'textual content'}
# Exposed modalities (all modalities from chunk level) as a property
md.modalities
>>> ['visual', 'textual']
# Implemented a method to extract content from modality (used in driver)
md.extract_content_from_modality('visual')
>>> 'visual content'

I personally think from_modality_content_mapping should be discussed further (keep or not) since it brings a bit inconsistency for extracting embedding or content.

JoanFM · 2020-11-23T22:17:13Z

jina/types/document/multimodal.py

+    """Each :class:`MultimodalDocument` should have at least 2 chunks (represent as :class:`DocumentSet`)
+    and len(set(doc.chunks.modality)) == len(doc.chunk)
+    """
+    def __init__(self, document = None,


Maybe there should be the possibility to build it from N documents with different modalities and to merge them into one multimodal document?

Would that also be useful?

Another useful interface would be to extract the embedding or content by modality (or the chunk) given a modality name.

We will need that interface, otherwise we do not remove any heavylifting from the user when creating this, no?. Or we will need some kind of MultiModalDocumentBuilder?

We need to decide what experience we want to offer when building a document like this

I think we can offer a Builder interface or design this class as a Builder. And delegate the checks of correctness at the last step of the build step

I had the same thought before, it should be something like an observer. The assurance of the correctness should happens at when we call chunks.add or chunks.append at DocumentSet or ChunkSet. But meanwhile I'm afraid it's a bit "over engineering". But it's good to have some discussion over that.

For now I would be happy just by adding thr interface to build directly from chunks. So that some boilerplatr can be added. What we would need to know is maybe about the granularities to be assigned and so on

the interface add as from_chunks. for check of the correctness, now I'm using an internal method called _validate, and chunks were validated at 2 places: 1. if we build MultimodalDocument using from_chunks or constructor, it will be validated inside the constructor. 2. Validate the chunks at modality_content_mapping inside _build_modality_content_mapping, since modality_content_mapping is the common entrance of other methods and properties. and we avoid overkill/change DocumentSet.add and ChunkSet.append

CHANGELOG.md

jina/drivers/multimodal.py

jina/types/document/multimodal.py

jina/drivers/multimodal.py

codecov · 2020-11-25T22:42:24Z

Codecov Report

Merging #1335 (c38e755) into master (6a77950) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1335      +/-   ##
==========================================
+ Coverage   83.52%   83.58%   +0.06%     
==========================================
  Files         103      104       +1     
  Lines        6792     6861      +69     
==========================================
+ Hits         5673     5735      +62     
- Misses       1119     1126       +7

Impacted Files	Coverage Δ
jina/drivers/multimodal.py	`91.42% <100.00%> (-3.93%)`	⬇️
jina/excepts.py	`100.00% <100.00%> (ø)`
jina/types/document/__init__.py	`97.36% <100.00%> (+0.05%)`	⬆️
jina/types/document/multimodal.py	`100.00% <100.00%> (ø)`
jina/types/sets/document_set.py	`98.85% <100.00%> (+0.13%)`	⬆️
jina/peapods/grpc_asyncio.py	`76.53% <0.00%> (-4.09%)`	⬇️
jina/logging/sse.py	`91.93% <0.00%> (-3.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a77950...c38e755. Read the comment docs.

jina/types/document/multimodal.py

JoanFM

Before merging I would like to have exposed an interface that lets the user build a MultiModalDocument from different data like:

d = MultiModalDocument({'modalitya': contentA, 'modalityB': contentB})

or

d = MultiModalDocument.from_content_or_embedding({'modalitya': contentA, 'modalityB': contentB}) (think about better naming)

Also I think that this class should have better care of setting the right granularity parameters for the MultiModalDocument and the chunks. (Not so important but needed for the sake of coherence)

JoanFM · 2020-11-29T07:17:33Z

jina/types/document/multimodal.py

+        self._modality_content_mapping = {}
+        if chunks:
+            self._validate(chunks)
+            self.chunks.clear()


Isn't it weird to clear something that has not been initialized?

JoanFM · 2020-11-29T07:18:31Z

jina/types/document/multimodal.py

+            self._build_modality_content_mapping()
+        return self._modality_content_mapping
+
+    def extract_content_by_modality(self, modality: str) -> DocumentContentType:


I would rather use from-modality since by seems like we are grouping

bwanglzu · 2020-11-29T21:17:25Z

@JoanFM how about

MultimodalDocument.from_content_modality_mapping({'visual': xxx, 'textual': xxx})

Where xxx could be content or embedding. this classmethod has the same naming convention as the property MultimodalDocument.content_modality_mapping

JoanFM · 2020-11-29T21:19:36Z

@JoanFM how about
MultimodalDocument.from_content_modality_mapping({'visual': xxx, 'textual': xxx}) 
Where xxx could be content or embedding. this classmethod has the same naming convention as the property MultimodalDocument.content_modality_mapping

This is what I meant yes. The problem we have is that is not easy to tell if the content when is a numpy array if it is embedding or content. So we need a flag to say that.

bwanglzu · 2020-11-29T21:22:18Z

Also I think that this class should have better care of setting the right granularity parameters for the MultiModalDocument and the chunks. (Not so important but needed for the sake of coherence)

Agreed, I'll create a getter & setter in :class:Document to expose granularity, now we can only access the property from :class:DocumentProto

JoanFM · 2020-11-29T21:28:52Z

@JoanFM how about
MultimodalDocument.from_content_modality_mapping({'visual': xxx, 'textual': xxx}) 
Where xxx could be content or embedding. this classmethod has the same naming convention as the property MultimodalDocument.content_modality_mapping
This is what I meant yes. The problem we have is that is not easy to tell if the content when is a numpy array if it is embedding or content. So we need a flag to say that.

Well i thought it twice and no, I think is better to assume they arr created from content so u assume the chunks are filled by document. But make sure it is documented that if one chunk is created from embeddings they will need to create from the other interface

nan-wang

LGTM👍

jina/types/document/multimodal.py

JoanFM

Still missing the handling of granularity level, but looks really good!

JoanFM · 2020-12-01T06:22:41Z

jina/types/document/multimodal.py

+                else chunk.content
+        self._validate(chunks=self.chunks)
+
+    def _validate(self, chunks: List[Document]):


just being a little picky, but we either make it static or we do not pass self.chunks and extract them inside the function right? I would vote for the second

jina/types/document/multimodal.py

jina/drivers/multimodal.py

* feat: add multimodal set * test: move conftest * test: fix unit test for multimodal driver

jina-bot added size/S area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality component/driver component/type labels Nov 23, 2020

JoanFM reviewed Nov 23, 2020

View reviewed changes

bwanglzu self-assigned this Nov 23, 2020

JoanFM reviewed Nov 24, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

JoanFM reviewed Nov 24, 2020

View reviewed changes

jina/drivers/multimodal.py Show resolved Hide resolved

jina/types/document/multimodal.py Outdated Show resolved Hide resolved

jina/drivers/multimodal.py Outdated Show resolved Hide resolved

bwanglzu added 2 commits November 25, 2020 18:21

feat: draft multimodal document

c8d9c47

feat: add draft multimodal document

c73e7f5

bwanglzu force-pushed the feat-add-multimodal-document branch from a10465a to c73e7f5 Compare November 25, 2020 17:24

feat: add multimodal document

5afb4b1

jina-bot added size/M area/testing This issue/PR affects testing and removed size/S labels Nov 25, 2020

bwanglzu added 3 commits November 25, 2020 22:50

feat: add multimodal document

e5cab6d

feat: add multimodal document

9512450

feat: add multimodal document

fdfce3b

bwanglzu marked this pull request as ready for review November 25, 2020 22:42

bwanglzu requested a review from a team as a code owner November 25, 2020 22:42

bwanglzu requested review from imsergiy and deepankarm and removed request for imsergiy November 25, 2020 22:42

bwanglzu changed the title ~~feat: draft multimodal document~~ feat: add multimodal document Nov 25, 2020

nan-wang reviewed Nov 26, 2020

View reviewed changes

jina/types/document/multimodal.py Show resolved Hide resolved

bwanglzu added 3 commits November 26, 2020 13:01

feat: add from chunks classmethod

366207c

feat: add from chunks classmethod

d1ec98b

feat: add from chunks classmethod

b90d1f8

feat: add from chunks classmethod

4c4f0c5

JoanFM requested changes Nov 29, 2020

View reviewed changes

nan-wang previously approved these changes Dec 1, 2020

View reviewed changes

jina/types/document/multimodal.py Show resolved Hide resolved

JoanFM reviewed Dec 1, 2020

View reviewed changes

feat: add multimodal document

de1a11b

bwanglzu dismissed nan-wang’s stale review via de1a11b December 1, 2020 22:03

feat: add multimodal document

2338282

JoanFM reviewed Dec 1, 2020

View reviewed changes

jina/types/document/multimodal.py Show resolved Hide resolved

feat: add multimodal document

c552097

JoanFM reviewed Dec 2, 2020

View reviewed changes

jina/drivers/multimodal.py Show resolved Hide resolved

bwanglzu and others added 3 commits December 2, 2020 16:36

feat: add multimodal document

1d8e34c

feat: add multimodal set (#1385)

04e20f3

* feat: add multimodal set * test: move conftest * test: fix unit test for multimodal driver

Merge branch 'master' into feat-add-multimodal-document

c38e755

JoanFM approved these changes Dec 2, 2020

View reviewed changes

JoanFM merged commit e68c7ad into master Dec 2, 2020

JoanFM deleted the feat-add-multimodal-document branch December 2, 2020 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add multimodal document #1335

feat: add multimodal document #1335

bwanglzu commented Nov 23, 2020 •

edited

Loading

JoanFM Nov 23, 2020

JoanFM Nov 23, 2020

JoanFM Nov 25, 2020

JoanFM Nov 25, 2020

bwanglzu Nov 25, 2020 •

edited

Loading

JoanFM Nov 25, 2020

bwanglzu Nov 26, 2020

codecov bot commented Nov 25, 2020 •

edited

Loading

JoanFM left a comment •

edited

Loading

JoanFM Nov 29, 2020

JoanFM Nov 29, 2020

bwanglzu commented Nov 29, 2020

JoanFM commented Nov 29, 2020

bwanglzu commented Nov 29, 2020

JoanFM commented Nov 29, 2020

nan-wang left a comment

JoanFM left a comment •

edited

Loading

JoanFM Dec 1, 2020

feat: add multimodal document #1335

feat: add multimodal document #1335

Conversation

bwanglzu commented Nov 23, 2020 • 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

bwanglzu Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2020 • edited Loading

Codecov Report

JoanFM left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwanglzu commented Nov 29, 2020

JoanFM commented Nov 29, 2020

bwanglzu commented Nov 29, 2020

JoanFM commented Nov 29, 2020

nan-wang left a comment

Choose a reason for hiding this comment

JoanFM left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwanglzu commented Nov 23, 2020 •

edited

Loading

bwanglzu Nov 25, 2020 •

edited

Loading

codecov bot commented Nov 25, 2020 •

edited

Loading

JoanFM left a comment •

edited

Loading

JoanFM left a comment •

edited

Loading