[None][fix] make bypass_processor_output_validation thread-safe - #14604
[None][fix] make bypass_processor_output_validation thread-safe#14604longlee0622 wants to merge 1 commit into
Conversation
The context manager patches module-level validate_typed_dict on transformers modules and restores on exit. Preprocessing is dispatched via asyncio.to_thread, so multiple workers can sit inside this context manager concurrently. Each entering thread captured the already-patched function as its "original" and installed a new wrapper on top, producing unbounded recursion (hundreds of nested _filtered_validate frames under video concurrency) and corrupting the restore on out-of-order exit. Reference-count the patch under a lock: the first entrant captures the true originals and installs a single shared wrapper, later entrants only bump a counter, and the last leaver restores. The wrapper itself is unchanged. Adds a unit test module exercising the concurrent-entry invariants (shared wrapper identity, bounded recursion depth, clean restore). Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesThread-safe validator bypass mechanism
Sequence DiagramsequenceDiagram
participant T1 as Thread 1
participant T2 as Thread 2
participant Lock as _bypass_lock
participant State as Module State
participant Binder
T1->>Lock: acquire()
Lock->>T1: locked
T1->>State: check _bypass_depth == 0
State->>T1: true
T1->>Binder: save original validate_typed_dict
T1->>State: save to _bypass_saved_originals
T1->>Binder: install _filtered_validate wrapper
T1->>State: set _bypass_depth = 1
T1->>Lock: release()
T2->>Lock: acquire()
Lock->>T2: locked
T2->>State: check _bypass_depth == 0
State->>T2: false
T2->>State: increment _bypass_depth to 2
T2->>Lock: release()
T1->>Binder: validator call (sees wrapper)
T2->>Binder: validator call (same wrapper)
T1->>Lock: acquire()
Lock->>T1: locked
T1->>State: decrement _bypass_depth to 1
T1->>State: check if == 0
State->>T1: false
T1->>Lock: release()
T2->>Lock: acquire()
Lock->>T2: locked
T2->>State: decrement _bypass_depth to 0
T2->>State: check if == 0
State->>T2: true
T2->>Binder: restore original validate_typed_dict
T2->>State: clear _bypass_saved_originals
T2->>Lock: release()
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary
bypass_processor_output_validation()intensorrt_llm/_torch/models/modeling_multimodal_utils.py(introduced in #12829) is thread-unsafe. Preprocessing for multimodal serving is dispatched viaasyncio.to_thread(self.llm.preprocess, ...), so multiple worker threads can sit inside this context manager concurrently. The original implementation captured each entering thread's view ofvalidate_typed_dictas its "original" and installed a new wrapper on top — under high concurrency this stacks wrappers indefinitely, producing unbounded recursion and corrupting the restore on out-of-order exit.The customer-reported symptom on Qwen3-VL / cosmos3-reasoner at 256-concurrency video serving was a recursion-limit traceback that returned HTTP 400 to the client; the traceback showed
_filtered_validaterepeating 968–975 times before reaching the realhuggingface_hub.dataclasses.validate_typed_dict.Fix
Reference-count the patch under a
threading.Lock:This also fixes the secondary "restore corruption" bug where Thread A's
finallycould restore the real function while Thread B's staleoriginalssnapshot would then re-patch with a dangling wrapper.Regression test
Added
tests/unittest/_torch/modeling/test_bypass_processor_output_validation.py. Five tests, no GPU / no model weights needed:test_originals_restored_after_exit— basic invariant.test_filter_strips_output_keys— confirms_PROCESSOR_OUTPUT_KEYSare filtered before reaching the validator.test_concurrent_entry_does_not_stack_wrappers— 64 threads enter behind aBarrier; all must observe the same wrapper object.test_concurrent_entry_does_not_recurse— directly pins down the reporter's recursion symptom: walkssys._getframe()to count_filtered_validateframes; with the fixmax(depths) == 1regardless of concurrency.test_sequential_reentry_restores_cleanly— guards the refcount state machine across repeated enter/exit.Test plan
tests/unittest/_torch/modeling/test_bypass_processor_output_validation.py)aiperf profile -m qwen3_vl --concurrency 256 ... --video-*againsttrtllm-serveno longer returns HTTP 400 withRecursionErrortest_modeling_multimodal.pypaths that usebypass_processor_output_validationstill passBackground
Reported by Aswin Visva — fails the cosmos3-reasoner model at high video concurrency in the R16 TRT-LLM release. Suggest cherry-picking to the 1.3 release branch once merged.
Summary by CodeRabbit
Tests
Improvements