-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RAG] Bumping up transformers version to 3.3.x #579
Conversation
…uggingface/transformers#1089 Signed-off-by: lalitpagaria <pagaria.lalit@gmail.com>
It seems model conversion test is failing but I am not able to pin point issue -
These changes might give some clue huggingface/transformers@v3.2.0...master (I don't suspect that changes in v3.3.0 or v3.3.1 causing this) |
@bogdankostic Can you please have a look at this failing model conversion test? As you are currently refactoring the conversion (#576) we should make sure that it also works for transformers==3.3.1. So probably we should merge #576 first, then rebase or merge here and fix it if still failing. |
I think I found the problem about the failing model conversion test. It seems to be related to this PR in transformers: huggingface/transformers#7272 I'm not completely sure about the purpose of the pooling layer, but in their PR they describe that it is not needed and in fact not even used. How should we proceed? |
Pooling layers are now removed for QA, NER and LM before converting FARM models to transformers in order to conform to the new transformers version. |
Great, I will run our QA performance benchmarks tomorrow. If they pass, we can merge. |
The QA accuracy benchmark shows some slight changes in performance. Especially the Evaluator drops by 0.7%:
|
Hmm... not a big diff but still wonder where this comes from. What do you think @Timoeller ? Merging and later trying to understand the small diff if we find some time? |
I am fine with merging. We will deep dive into QA preds checking and other QA related stuff anyways soon. How about merging this monday morning or do you depend on it now? |
Monday is fine |
Can we please merge this and create new Farm version. Not able to properly run transformers and haystack together even after many dirty hacks.
|
Towards deepset-ai/haystack#443
@tholor Please review