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

refactor: remove Inferencer multiprocessing #3283

Merged
merged 4 commits into from
Oct 4, 2022
Merged

refactor: remove Inferencer multiprocessing #3283

merged 4 commits into from
Oct 4, 2022

Conversation

vblagoje
Copy link
Member

Related Issues

Proposed Changes:

Inferencer multiprocessing just as in #3087 started to cause lockups recently due to various inconsistencies around multiprocessing in torch. Some torch versions worked well, while others caused outright deadlocks.

As there are no performance benefits of multiprocessing, it is best to remove it altogether. Several examples were independently made on how multiprocessing slows down inferencing.

How did you test it?

CI tests, performance tests such as this colab notebook

Notes for the reviewer

TBD

Checklist

@vblagoje vblagoje requested a review from a team as a code owner September 27, 2022 07:57
@vblagoje vblagoje requested review from masci and removed request for a team September 27, 2022 07:57
@vblagoje vblagoje mentioned this pull request Sep 27, 2022
@sjrl
Copy link
Contributor

sjrl commented Sep 28, 2022

Hi, @vblagoje thanks for the work! An issue recently opened by @danielbichuetti may also be related to this, which can be found here Issue #3289. It's possible that the multiprocessing in the FARMReader is preventing the multiprocessing in the Inferencer from working as expected. Edit: Sorry I misunderstood, the multiprocessing in the FARMReader and the Inferencer are the same.

Would it be possible to do the same CI performance check using the TransformerReader instead and see if this time difference is still observed? Edit: Actually, this isn't really relevant since the TransformerReader does not use the same Inferencer as the FARMReader so it will not have this multiprocessing performance problem.

@vblagoje
Copy link
Member Author

Yeah totally-have a look at the notebook I made to measure FarmReader performance with/without multiprocessing. It should be relatively simple to create TransformerReader and inference it instead of FarmReader. I'll do it tomorrow. We should also do measurements on CPU as well for both versions.

@sjrl
Copy link
Contributor

sjrl commented Sep 28, 2022

@vblagoje Yeah sounds good! Also, I'm in the US right now hence why I'm still online so please feel free to respond tomorrow at a more reasonable time in Berlin!

@sjrl
Copy link
Contributor

sjrl commented Sep 28, 2022

@vblagoje See comment #3272 (comment) on the original issue opened by @Timoeller.

@masci masci changed the title Remove Inferencer multiprocessing refactor: remove Inferencer multiprocessing Sep 29, 2022
@vblagoje vblagoje requested a review from sjrl September 29, 2022 17:10
@sjrl
Copy link
Contributor

sjrl commented Sep 30, 2022

@vblagoje I think it is the correct move to remove multiprocessing from the inferencer since it seems to conflict with multiprocessing from the FastTokenizers in HF.

My only suggestion would be to go through infer.py and add deprecation warnings to all places where the variable multiprocessing_chunksize appears since it no longer used. For example in functions like inference_from_file, inference_from_objects, etc. in both the class Inferencer and the class QAInferencer.

@vblagoje
Copy link
Member Author

vblagoje commented Oct 4, 2022

@sjrl @masci let me know if we need to do anything else for this one

except AttributeError:
pass
yield from predictions

@classmethod
def _create_datasets_chunkwise(cls, chunk, processor: Processor):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vblagoje Should the method _create_datasets_chunkwise also be removed? It was only used in _inference_with_multiprocessing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 spot on, removed @sjrl

Copy link
Contributor

@sjrl sjrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vblagoje vblagoje merged commit 6cb4e93 into deepset-ai:main Oct 4, 2022
@vblagoje vblagoje deleted the fix_qa_mp_inferencing branch October 24, 2022 08:20
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 this pull request may close these issues.

QA inferencer very slow because of bad default multiprocessing settings
2 participants