fix(inference): fix concurrent request Rust borrow checker panic - #5733
Conversation
also added an integration test exposing the issue
Pull request was converted to draft
this change exposes the issue ogx-ai#5752
|
Hey @leseb, I think that these tests passed this time because I was just lucky. I have a fix for the Issue #5752 (which will periodically strike with this test using threads to make requests in parallel), though the fix is limited to the Without it, I'm afraid that the test sending the requests in parallel will be flaky. Asking because I also just saw your PR #5760 raising deprecation warnings. |
mattf
left a comment
mattf
left a comment
There was a problem hiding this comment.
@zy1o this isn't your fault but we should delete this rerank code instead of making it thread safe. though fixing it would be nice. issue off the top of my head -
- (small) redundant checks on request.items / request.max_num_results
- (big) qwen specific prompts
- (big) use of casual lm on a cross encoder model
- (medium) liberal use of
Any
@franciscojavierarceo i wonder if this impacts your rag benchmarks. this code is going to produce nonsense w/o a qwen model and maybe even with, since it treats a cross encoder model as a casual.
|
@mattf I am new to this code base, but less code is a win in my book. I was looking at parallelizing test execution when i stumbled over these concurrency issues. Feel free to discard this PR. 🍻 |
welcome and thank you for looking into these things. given we have a sentence_transformer inference provider as well, which lacks a rerank impl, we should delete this and add a rerank impl to SentenceTransformersInferenceImpl. |
@mattf would you want me to take care of the move? It feels like this change will have a bigger blast radius. It feels that the Issue #5732 should also be addressed. I have a feeling that it will surface at a random point in time and cause confusion. My proposal is to continue as follows:
|
i'm not too concerned about that, because it'd be a circular import and explode pretty early, but welcome improvements.
brilliant! |
|
Hey @mattf, @leseb, @franciscojavierarceo Now that #5773 was merged in, the path to Step 2 (merging this and clearing the borrow checker panic) is clear. All checks are green! |
# What does this PR do? * Moves `rerank` implementation from `transformers` to `sentence_transformers` - following up on @mattf's [comment](#5733 (comment)). * Removes `transformers` module from inline providers, as it was simply empty after the move. * Updates benchmarking k8s scripts. * Updates distrubutions. * Updates unit tests and integration tests accordingly. Also search & replace pre-recorded responses to match `sentence-transformers` provider. Simply updated: ``` provider_id="transformers", model_id="Qwen/Qwen3-Reranker-0.6B", ``` to ``` provider_id="sentence-transformers", model_id="Qwen/Qwen3-Reranker-0.6B", ``` across all files. Closes #5732. **Notes** * This PR does not address other issues raised by @mattf in this [comment](#5733 (review)), I can pick them up, once small step at a time. * I noticed that the `integration-tests.sh` has the models hardcoded: ``` # Run Python tests unless typescript-only mode if [[ "$TYPESCRIPT_ONLY" == "false" ]]; then pytest -s -v $PYTEST_TARGET \ $STACK_CONFIG_ARG \ --inference-mode="$INFERENCE_MODE" \ -k "$PYTEST_PATTERN" \ $EXTRA_PARAMS \ --color=yes \ --embedding-model=sentence-transformers/nomic-ai/nomic-embed-text-v1.5 \ --rerank-model=sentence-transformers/Qwen/Qwen3-Reranker-0.6B \ --capture=tee-sys exit_code=$? else echo "Skipping Python tests (--typescript-only mode)" exit_code=0 fi ``` and I think that is should be: ``` --embedding-model="${OGX_TEST_EMBEDDING_MODEL:-sentence-transformers/nomic-ai/nomic-embed-text-v1.5}" \ --rerank-model="${OGX_TEST_RERANK_MODEL:-sentence-transformers/Qwen/Qwen3-Reranker-0.6B}" \ ``` ## Test Plan ``` ./scripts/integration-tests.sh --stack-config server:ci-tests \ --inference-mode replay --setup vllm --suite base-vllm-subset ``` --------- Signed-off-by: Charlie Doern <cdoern@redhat.com> Co-authored-by: Derek Higgins <derekh@redhat.com> Co-authored-by: Francisco Javier Arceo <arceofrancisco@gmail.com> Co-authored-by: Charlie Doern <cdoern@redhat.com> Co-authored-by: Matthew Farrellee <matt@cs.wisc.edu> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
4 participants
also added an integration test exposing the issue
What does this PR do?
Wrap inline reranker call in a
threading.Lockpreventing threads from changing a mutable Rust object at the same time resulting with borrow checker panic.Closes #5729
Test Plan
Added an integration test exposing this problem which seems to be invoking the bug every time on my local dev machine, so executing the tests should suffice.
With the lock applied, the test passes reliably.
Re-using the
test_rerank_textlogic (just above) just sending all the sequential requests at the same time(-ish) to trigger this race condition. UsingThreadPoolExecutorbecause the existing fixtureclient_with_modelsexposes the sync client.