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

Fix and document Inferencer usage and pool handling #429

Merged
merged 7 commits into from Jul 23, 2020

Conversation

PhilipMay
Copy link
Contributor

@PhilipMay PhilipMay commented Jun 26, 2020

This is the 2nd part of PR #403

TODO

  • check examples
  • check call in fit_s3e_on_corpus
  • check other documentation
  • sould other code be changed to close pool
  • must be merged with PR Fix bug in benchmark tests with if statement #430
  • update docs/basic_usage.rst
  • update docs/examples.rst
  • update tutorials/1_farm_building_blocks.ipynb
  • write explicit example in doc and tell the user how to use this

@tanaysoni
Copy link
Contributor

Hi @PhilipMay, I added a suggestion commit with a call to the close_multiprocessing_pool() each time we use the Inferencer. In that case, we might not need the warning comments. What do you think?

@PhilipMay
Copy link
Contributor Author

Hi @PhilipMay, I added a suggestion commit with a call to the close_multiprocessing_pool() each time we use the Inferencer. In that case, we might not need the warning comments. What do you think?

Well, that does not fully work because you would have to wrap the code into a try and finally block to even close the pool when an exception is raised.

@PhilipMay
Copy link
Contributor Author

And to avoid this kind of "ugly and complicated" code I would better not open a pool... So the example is easy to read.

@tanaysoni
Copy link
Contributor

Hi @PhilipMay,

In the case of exception in the Inferencer, the example scripts would crash, terminating all the Python subprocesses. For instance, the following script should exit clean without any zombie processes.

results = inferencer.inference_from_dicts(qa_input)
print(results)
raise Exception  # scripts exits here
inferencer.close_multiprocessing_pool()

@PhilipMay
Copy link
Contributor Author

@tanaysoni Yes I agree. In the example it does not cause any direct problems. But IMO an example should be something that can be taken and put into something bigger. If placed in a real life server environment this might cause an ugly memory leak if not handled correctly. But we already had this discussion on an other PR if I remember correctly.

@PhilipMay
Copy link
Contributor Author

So what do you suggest we can do with this uncompleted PR? In which direction should we go?

@tanaysoni
Copy link
Contributor

Hi @PhilipMay, IMO we add a call to close_multiprocessing_pool() in each example along with a single line comment mentioning it's important to close the pool. Additionally, we can write a detailed example explaining more details about the Pool behavior. What do you think?

@tanaysoni
Copy link
Contributor

Hi @PhilipMay, thanks again for bringing this up and working on it! I added an additional example inferencer_multiproccesing.py and rebased the branch with master. If you think there's more documentation we could add, let's discuss and take them up in a separate PR.

@tanaysoni tanaysoni merged commit a7ad670 into deepset-ai:master Jul 23, 2020
@PhilipMay
Copy link
Contributor Author

Ok. Thanks. 👍

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.

None yet

2 participants