Skip to content

fix: move to thread paralellism to avoid broken jvm fork state - #676

Open
vlcfaria wants to merge 1 commit into
terrier-org:masterfrom
vlcfaria:fix-gridsearch-jobs
Open

vlcfaria wants to merge 1 commit into
terrier-org:masterfrom
vlcfaria:fix-gridsearch-jobs

Conversation

@vlcfaria

Copy link
Copy Markdown

This is a bit of a rough attempt to fix GridScan on multiprocessing scenarios.

My understanding of the bug was that pyterrier_alpha.parallel._parallel_lambda_joblib uses process-level parallelism, which copies the JVM started flag and the JVM state which can be messy.

I swapped into thread-based parallelism, following Retriever.transform which I believe shares the JVM more cleanly while maintaining actual parallelism since they release the GIL. Ive kept the ray backend because it was kind of a grey area.

@cmacdonald

Copy link
Copy Markdown
Contributor

thanks @vlcfaria for this. We dont use the parallel stuff much, which is why it moved to alpha.

Can you explain a bit about your use case?

# instead of calling fork() and leaving leave the JVM in a weird state
def _eval_chunk(chunk):
out = [_evaluate_one_setting(k, v) for k, v in chunk]
detach() # release JNI refs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flake says we should be decorating this method with @pt.java.required, but I think the actual point is that if the transformer doesnt involve Java (Terrier or Anserini) then jnius may not even be installed. So we need to detect jnuis (try import etc) and act accordingly.

out = [_evaluate_one_setting(k, v) for k, v in chunk]
detach() # release JNI refs
return out
with ThreadPoolExecutor(max_workers=jobs) as ex:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you tested that you get a speed up with different Python threads calling Java? I think Terrier's data structures arent thread safe unless they are loaded with "concurrent:" prefix. At least with the forked JVM, if setup correctly, the results would be correct.

@cmacdonald

Copy link
Copy Markdown
Contributor

Can you explain a bit about your use case?

Ah, I see the issues now. Usecase is clear.

I have some questions that I have placed in the review.

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.

2 participants