deepset-ai / FARM

:house_with_garden: Fast & easy transfer learning for NLP. Harvesting language models for the industry. Focus on Question Answering.
https://farm.deepset.ai
Apache License 2.0
1.73k stars 247 forks source link

Fix issues 811 812 819 823 #825

Closed johann-petrak closed 3 years ago

johann-petrak commented 3 years ago

This should fix the following issues:

The following notes are important:

I have not tested this with more complex multihead models - yet: i will use this myself with multihead models, so should there be issues with that I will correct them then.

Timoeller commented 3 years ago

Thanks for combining the contributions into one very dense PR.

I think the addition of the ConcatTensorDataset might have broken something for other tasks, see the failing CI. Could you look into it? Maybe separate the additional ConcatTensorDataset from other tasks as a workaround?

Disclaimer: I will only be available for reviewing further adjustments from Monday onwards again.

johann-petrak commented 3 years ago

Sorry this was really caused by a stupid typo I introduced when I tried to avoid a warning message being too long in a single line.

The ConcatTensorDataset has been implemented as a subclass of ConcatDataset and literally works exactly the same unless one requests a slice or list of indices instead of a single index - which can never happen in any code not added by me here, because that would have been impossible so far.

I did run the tests here of which one fails: test_ner_amp.py::test_ner_amp but I suspect due to completely different/unrelated reasons. The tests in github actions seem to pass now: https://github.com/deepset-ai/FARM/runs/3013430217?check_suite_focus=true

Timoeller commented 3 years ago

btw as slight improvement for the next PR please mention "fixes issue num" explicitly in the PR description, then the corresponding issues get closed automatically.