opened
ftesser
username_0
This PR is related to #636.
It will be an incremental PR, since I see that after this first commit, some others changes are needed to successfully run all the tests on Windows.

This first commit remove  the not used 'from torch.distributed import all_gather' from 'farm\modeling\prediction_head.py'. There is also a newline added here.
Make tests working on Windws
issue
created
ftesser
username_0
E       UnicodeDecodeError: 'charmap' codec can't decode byte 0x8d in position 1012: character maps to <undefined>

c:\python\lib\encodings\cp1252.py:23: UnicodeDecodeError
```

So the second commit is need to fix that.
comment
created
ftesser
username_0
After the first two commits, there is only one test that fail on Windows:  test.test_dpr.test_dpr_modules
The reason is: 
```
E           AttributeError: module 'torch.distributed' has no attribute 'get_rank'

..\venv\lib\site-packages\farm\modeling\prediction_head.py:1644: AttributeError
```
I see that this refer to `logits_to_loss` method on  prediction_head.py :


```python
# Check if DDP is initialized
        try:
            rank = torch.distributed.get_rank()
        except AssertionError:
            rank = -1
```

Any suggestions to make this working also on windows test? 
If DDP does not work on windows perhaps we can exclude this test when running on Windows?
comment
created
ftesser
username_0
My current proposal is to exclude `test.test_dpr.test_dpr_modules` just when pytest is run on a Windows machine.
If you think that it is possible somehow to test `dpr_modules` without `DDP` it would probably be better, but for the moment this solution allows you to test almost everything even on windows.

Let me know what do you think.
comment
created
Timoeller
username_1
Hey @username_0 thanks for looking into this.
Generally I believe it doesnt make much sense having different code/tests for different distributions. It will be tedious to keep track of all the individual changes.

The DDP module is quite important for making DPR trainable so excluding it would not be preferred.

Looking into Pytorch support for DDP in windows I found this PR: https://github.com/pytorch/pytorch/pull/45335 which was merged on Sep 25th. So this code is only in the newest pytorch 1.7.0 release (we are currently using pytorch 1.6.0).

So actionable insights: We want to do a FARM release later this week, after this we can increment the pytorch version and see if this fixes the DDP issues. Would that be a solution for you? Of course you can try updating pytorch beforehand yourself, we would appreciate your insights.
comment
created
ftesser
username_0
Thanks @username_1 for your feedback.
Of course if with pytorch 1.7.0 DDP will works on Windows this is the best solution, so I am ok to wait for that. I will try to updating pytorch beforehand myself.

Just a note about this PR, commit https://github.com/deepset-ai/FARM/pull/637/commits/0aedd051e4f80dcefd9948cfba7e31ae37e0384c solve a UnicodeDecodeError, this should be applied in all cases.
comment
created
Timoeller
username_1
Sorry for the delay @username_0
We actually made huge changes in #649 that caused a lot of testing.

We also quickly tested updating to pytorch in #660 which produced failing onnx conversion tests. Unfortunately we will be able to look into this with the start of the new year.

I will merge you PR now to include this nice patch in the coming release.
comment