feat: add multimodal set #1385
feat: add multimodal set #1385
Conversation
jina-bot
added
size/M
area/core
JoanFM
force-pushed
the
fix-integration-test
branch
from
a4b3e70
to
1680725
Compare
Codecov Report
@@ Coverage Diff @@
## feat-add-multimodal-document #1385 +/- ##
================================================================
- Coverage 83.75% 83.25% -0.51%
================================================================
Files 104 104
Lines 6921 6884 -37
================================================================
- Hits 5797 5731 -66
- Misses 1124 1153 +29
Continue to review full report at Codecov.
|
JoanFM
force-pushed
the
fix-integration-test
branch
from
961432e
to
689d328
Compare
JoanFM
force-pushed
the
fix-integration-test
branch
from
689d328
to
7728cdc
Compare
* feat: draft multimodal document * feat: add draft multimodal document * feat: add multimodal document * feat: add multimodal document * feat: add multimodal document * feat: add multimodal document * feat: add from chunks classmethod * feat: add from chunks classmethod * feat: add from chunks classmethod * feat: add from chunks classmethod * feat: add multimodal document * feat: add multimodal document * feat: add multimodal document * feat: add multimodal document * feat: add multimodal set (#1385) * feat: add multimodal set * test: move conftest * test: fix unit test for multimodal driver Co-authored-by: Joan Fontanals <joan.martinez@jina.ai>
| @@ -88,3 +88,21 @@ def build(self): | |||
|
|
|||
| def sort(self, *args, **kwargs): | |||
| self._docs_proto.sort(*args, **kwargs) | |||
|
|
|||
|
|
|||
| class MultiModalDocumentSet(DocumentSet): | |||
There was a problem hiding this comment.
@JoanFM We need a consistent naming here. MultiModalDocumentSet and MultimodalDocument is not a good choice.
There was a problem hiding this comment.
Oh yes I will change it, what is preferred?
There was a problem hiding this comment.
Personally, MultimodalDocumentSet is prefered because multimodal is one word.
There was a problem hiding this comment.
In case, we might have a CrossModalDocumentSet in the future, I'd prefer MultiModalDocumentSet. But the cross-modal thingy might not make to much sense.
There was a problem hiding this comment.
I think there will not be a CrossModalDocumentSet
There was a problem hiding this comment.
Opened a PR to fix this #1390
| def validate_response(resp): | ||
| is_callback_called._callback_called = True |
There was a problem hiding this comment.
This is overkill and not the right way. We should use the build in and standardized way of doing it. There are two ways. Either we use unittest.mock.Mock like:
response_mock = Mock(wraps=validate_response)
index_gt_flow.index(input_fn=multimodal_all_types_documents, output_fn=response_mock)
mock.assert_called()
Or we add pytest-mock as a dependency and do:
def test_multimodal_all_types_parallel(multimodal_all_types_documents, mocker):
...
response_mock = mocker.Mock(wraps=validate_response)
index_gt_flow.index(input_fn=multimodal_all_types_documents, output_fn=response_mock)
mock.assert_called()
I'd prefer using unittest.mock.Mock.
There was a problem hiding this comment.
I am working on it in general in another PR will try to implement ur suggestions, is there a way to have it easily included in the tests?
There was a problem hiding this comment.
It is exactly as easy as I wrote. just mock the callback function and test with mock.assert_called() if the callback was called.
There was a problem hiding this comment.
yes, yes. I mean something more automatic without much effort, But I think there isnot. About to change in #1391
No description provided.