Skip to content
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

Remove quick fix to avoid applying DataParallel twice? #1093

Closed
julian-risch opened this issue May 25, 2021 · 2 comments · Fixed by #1196
Closed

Remove quick fix to avoid applying DataParallel twice? #1093

julian-risch opened this issue May 25, 2021 · 2 comments · Fixed by #1196
Assignees

Comments

@julian-risch
Copy link
Member

Is your feature request related to a problem? Please describe.
Issue #224 reported problems with a multi-GPU setup and #234 introduced a quick fix. However, commenting out the fix, I cannot reproduce the earlier problems anymore.

Describe the solution you'd like
I wonder if the quick fix to avoid applying DataParallel twice can be removed by deleting the following three lines of code in reader and ranker nodes:

self.inferencer.model.save("tmp_model")
model = BaseAdaptiveModel.load(load_dir="tmp_model", device=device, strict=True)
shutil.rmtree('tmp_model')

Throughout the rest of the code, model should be replaced with self.inferencer.model if these lines are removed.

Additional context
Having apex installed on a machine with 4 GPUs and running tutorial 5 with python -m torch.distributed.launch , I couldn't find any difference in the logging output with or without the quick fix. It seems to run fine but I have never used apex before so I might have overlooked something. I could not find a check in FARM's optimize_model() that avoids applying DataParallel there if it was already done before: https://github.com/deepset-ai/FARM/blob/816b4e3e65c142f8a31a63833058b75fe0419ed4/farm/modeling/optimization.py#L272

@julian-risch
Copy link
Member Author

With Timo I just saw that the optimize_model method is not called anymore in the Inferencer in FARM but the import is still there. @tholor your commit here removed the call of optimize_model in the Inferencer. That's why I could not reproduce the error, I guess. deepset-ai/FARM@94c6b8d#diff-8a98ef73f1fa756ee7fdc0cd3a2f8bdc07f4e2eb490111356337ddc8c170066cR278

@tholor
Copy link
Member

tholor commented Jun 14, 2021

@julian-risch Ok, so let's remove the lines in Haystack then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants