fix: ONNX FARMReader model conversion is broken#3211
fix: ONNX FARMReader model conversion is broken#3211vblagoje merged 10 commits intodeepset-ai:mainfrom vblagoje:fix_onnx
Conversation
vblagoje
added this to the
https://github.com/deepset-ai/haystack/milestone/8 milestone
There was a problem hiding this comment.
You might use here this function:
There was a problem hiding this comment.
Will try, thanks Bogdan
|
@bogdankostic when I move the onnx test to be executed after test_retrieval (mdr+memory) then both tests pass. |
Create new onnx tests, add first test
|
@bogdankostic with your suggested change this PR started to pass 🚀 |
|
I added dedicated onnx test file so we can start testing this part of the code base even better in the future |
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
LGTM, just added some minor comments about imports that don't seem to be used.
test/nodes/test_retriever.py
Outdated
| @@ -1,4 +1,5 @@ | |||
| import logging | |||
| import os | |||
There was a problem hiding this comment.
I think this import is not used either.
test/nodes/test_retriever.py
Outdated
| from elasticsearch import Elasticsearch | ||
|
|
||
| from haystack.document_stores import WeaviateDocumentStore | ||
| from haystack.nodes import FARMReader |
test/nodes/test_reader.py
Outdated
| @@ -1,4 +1,7 @@ | |||
| import math | |||
| import os | |||
| import tempfile | |||
There was a problem hiding this comment.
I think this import is not used.
There was a problem hiding this comment.
Aha, that's because we don't check imports for tests, will fix - thanks @bogdankostic
2 participants
Related Issues
Proposed Changes:
During a recent language and tokenization refactoring, we broke FarmReader ONNX conversion support. A simple ONNX conversion of a QA model fails; we don't have a test case for it.
How did you test it?
Added a unit test for FarmReader ONNX conversion
Notes for the reviewer
Is there a more straightforward way to figure out the model type (aside from loading it using
get_language_model)? See PR changes.Checklist