Skip to content

fix(sdk): stop serializing concurrent LLM calls through modify_params lock - #4447

Draft
santhiprakash wants to merge 1 commit into
OpenHands:mainfrom
santhiprakash:fix/llm-modify-params-sync-serialization
Draft

fix(sdk): stop serializing concurrent LLM calls through modify_params lock#4447
santhiprakash wants to merge 1 commit into
OpenHands:mainfrom
santhiprakash:fix/llm-modify-params-sync-serialization

Conversation

@santhiprakash

Copy link
Copy Markdown

HUMAN:


AGENT:

Why

A ClassVar[threading.Lock] in openhands/sdk/llm/llm.py was held for the entire duration of each LLM transport call to guard the process-wide litellm.modify_params boolean. Because it was a ClassVar, the lock was shared across all LLM instances in the process, serializing all concurrent sync conversations. With local models (vLLM, Ollama, llama.cpp) where a single call takes 30-120s, 3 parallel conversations took 90-360s instead of 30-120s — the GPU sat idle while other conversations waited on the lock.

litellm.modify_params is read by LiteLLM synchronously during request transformation (before the HTTP call) — in prompt_templates/factory.py, utils.py, and provider-specific transformation modules. Once the HTTP request is sent, the flag is no longer consulted. The lock therefore only needs to guard the set/restore of the global, not the full HTTP round-trip.

Summary

  • Replace the hold-for-full-duration lock with a reference-counted set/restore: the lock is held only for the brief counter update and global set/restore (microseconds), then released so concurrent sync and async calls overlap. When the last in-flight call finishes, the global is restored to its pre-call value.
  • Simplify the async path (_alitellm_modify_params_ctx): the previous run_in_executor / asyncio.shield / cancellation done-callback machinery (added in fix: don't freeze agent-server's event loop while waiting on litellm's global modify_params lock #4012 to avoid event-loop freezes) is no longer needed because the lock is no longer held across the await. A plain blocking with is safe when the lock is held for microseconds.
  • Remove the now-unused _litellm_modify_params_lock_executor ThreadPoolExecutor.

Race note: if two concurrent calls request different modify_params values, the last entrant wins the global during transformation. This is inherent to LiteLLM's process-wide global and cannot be solved without an upstream LiteLLM change. In practice, concurrent calls in the same process almost always share the same modify_params value, and local models (where the serialization hurts most) use modify_params=False.

Issue Number

Fixes #16459.

How to Test

# Run the modify_params context-manager tests
uv run pytest tests/sdk/llm/test_llm_completion.py -k "modify_params" -v

# Run the full LLM test suite
uv run pytest tests/sdk/llm/ -v

# Lint / format / type check
uv run ruff check openhands-sdk/openhands/sdk/llm/llm.py tests/sdk/llm/test_llm_completion.py
uv run ruff format --check openhands-sdk/openhands/sdk/llm/llm.py tests/sdk/llm/test_llm_completion.py
uv run pre-commit run --files openhands-sdk/openhands/sdk/llm/llm.py tests/sdk/llm/test_llm_completion.py

Verification results

$ uv run pytest tests/sdk/llm/test_llm_completion.py -k "modify_params" -v
tests/sdk/llm/test_llm_completion.py::test_litellm_modify_params_ctx_allows_concurrent_threads PASSED
tests/sdk/llm/test_llm_completion.py::test_litellm_modify_params_ctx_restores_original_after_all_done PASSED
tests/sdk/llm/test_llm_completion.py::test_litellm_modify_params_ctx_ref_count_nested PASSED
tests/sdk/llm/test_llm_completion.py::test_litellm_modify_params_ctx_concurrent_overlap PASSED
tests/sdk/llm/test_llm_completion.py::test_alitellm_modify_params_ctx_cleans_up_refs_on_cancel PASSED
tests/sdk/llm/test_llm_completion.py::test_alitellm_modify_params_ctx_does_not_block_event_loop PASSED
6 passed

$ uv run pytest tests/sdk/llm/ -v
949 passed

$ uv run pre-commit run --files openhands-sdk/openhands/sdk/llm/llm.py tests/sdk/llm/test_llm_completion.py
All checks passed (ruff format, ruff lint, pycodestyle, pyright, import deps, tool registration)

Key new test: test_litellm_modify_params_ctx_concurrent_overlap

Two concurrent sync calls each sleep for 0.3s inside the context manager. If serialized (old behavior), total wall time ≈ 0.6s. With the fix, both overlap → total ≈ 0.3s. The test asserts elapsed < 2 × call_duration to verify non-serialization.

Video/Screenshots

N/A — internal concurrency fix, no UI change.

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

  • The release-note-required label on the issue is about API breakage detection. This PR does not change any public API — _litellm_modify_params_ctx and _alitellm_modify_params_ctx are private methods. The ClassVar _litellm_modify_params_lock_executor is removed, but it was also private.
  • PR fix: don't freeze agent-server's event loop while waiting on litellm's global modify_params lock #4012 (merged 2026-07-07) fixed the async event-loop freeze by offloading lock acquisition to run_in_executor. This PR supersedes that approach by eliminating the long-held lock entirely, making the run_in_executor workaround unnecessary.

… lock

The class-level threading.Lock guarding litellm.modify_params was held for
the entire duration of each LLM transport call. Because it was a ClassVar,
the lock was shared across all LLM instances in the process, serializing
all concurrent sync conversations. With local models (vLLM, Ollama,
llama.cpp) where one call takes 30-120s, N parallel conversations took
N×30-120s instead of 30-120s — the GPU sat idle while other conversations
waited on the lock.

litellm.modify_params is a process-wide global that LiteLLM reads
synchronously during request transformation (before the HTTP call), so
the lock only needs to guard the set/restore of that global — not the
full HTTP round-trip.

Replace the hold-for-full-duration lock with a reference-counted
set/restore: the lock is held only for the brief counter update and
global set/restore, then released so concurrent calls overlap. When the
last in-flight call finishes, the global is restored to its pre-call
value.

The async path is simplified correspondingly: the previous
run_in_executor / shield / cancellation-callback machinery is no longer
needed because the lock is held for microseconds, not the full call
duration.

Race note: if two concurrent calls request different modify_params
values, the last entrant wins the global during transformation. This is
inherent to LiteLLM's process-wide global and cannot be solved without
an upstream LiteLLM change; in practice concurrent calls in the same
process almost always share the same value, and local models (where the
serialization hurts most) use modify_params=False.

Fixes #16459.
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.

1 participant