Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
e4ac21a to
7c9a1aa
Compare
561cfb5 to
733a786
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8dce53f to
5455d84
Compare
666abc3 to
137861d
Compare
…sizing FlashInfer allocates its shared float arena and each wrapper's dedicated int workspace lazily, on the first plan. Memory profiling runs before any of that, so KV cache sizing is told the activation peak of a model whose attention workspace does not exist yet, and the allocation lands afterwards -- against memory KV sizing has already handed out, and in the CUDA graph case against an arena execution has locked. Give the builder a lifecycle the runners can drive. A builder declares `PersistentWorkspaceProfilingSupport`, defaulting to `UNSUPPORTED` so an unknown builder in a mixed-backend model cannot be assumed neutral. When every builder of a model opts in, the runner reserves before the profiling measurement, holds the builders and their wrappers alive through it with a lease, rebinds them once the arenas are final, and locks the arena after the last warmup that could still size it. A model that does not opt in keeps the legacy path exactly, including no new lock: reserving nothing and then locking would turn its later lazy allocation into a hard failure. FlashInfer reserves conservatively: grow the shared arena to the default size it would have reached anyway, then build the wrappers the active prefill and decode routes will actually use, so their int workspaces are allocated inside the measured window. Direct trtllm-gen's module-global workspace is materialized the same way. Nothing here computes a size -- the point is only that the allocations happen where profiling can see them, which can lower the KV token budget by the amount the runtime used to take behind its back. GDN declares NEUTRAL so a hybrid model can still take the path. Signed-off-by: lesj0610 <lesj0610@users.noreply.github.com>
…sizing FlashInfer allocates its shared float arena and each wrapper's dedicated int workspace lazily, on the first plan. Memory profiling runs before any of that, so KV cache sizing is told the activation peak of a model whose attention workspace does not exist yet, and the allocation lands afterwards -- against memory KV sizing has already handed out, and in the CUDA graph case against an arena execution has locked. Give the builder a lifecycle the runners can drive. A builder declares `PersistentWorkspaceProfilingSupport`, defaulting to `UNSUPPORTED` so an unknown builder in a mixed-backend model cannot be assumed neutral. When every builder of a model opts in, the runner reserves before the profiling measurement, holds the builders and their wrappers alive through it with a lease, rebinds them once the arenas are final, and locks the arena after the last warmup that could still size it. A model that does not opt in keeps the legacy path exactly, including no new lock: reserving nothing and then locking would turn its later lazy allocation into a hard failure. The lease has to outlive the whole `memory_profiling` block, not just the profile run inside it. Only the shared arena is held by the global manager; the int workspace belongs to its wrapper alone, so releasing any earlier frees it before the closing measurement `total_consumed` comes from. It is released right after, before CUDA graph profiling rebuilds the minimal KV state. FlashInfer reserves conservatively: grow the shared arena to the default size it would have reached anyway, then build the wrappers the active prefill and decode routes will actually use, so their int workspaces are allocated inside the measured window. Direct trtllm-gen's module-global workspace is materialized the same way. Nothing here computes a size -- the point is only that the allocations happen where profiling can see them, which can lower the KV token budget by the amount the runtime used to take behind its back. That reservation owns the causal prefill wrapper, so a model whose prefill also reaches a wrapper it does not own has to stay out: mm-prefix answers `UNSUPPORTED` until the change that gives the reservation ownership of that wrapper opts it back in. GDN declares `NEUTRAL` so a hybrid model can still take the path. Signed-off-by: lesj0610 <lesj0610@users.noreply.github.com>
137861d to
d90f87a
Compare
Purpose
A FlashInfer wrapper allocates its dedicated int workspace on its first
plan(), and the shared float arena grows on demand, so on a fresh engine neither exists yet whendetermine_available_memory()runs. The closing measurement ofmemory_profilingtherefore does not see them, and KV cache sizing is computed without them.This PR adds a lifecycle that materializes that workspace inside the profiling window, keeps it alive with a lease until the closing measurement has read it, and locks the shared arena once the last warmup that could still size it has run.
AI assistance: Codex, Claude
What this changes
Three contracts.
Fail-closed support declaration.
AttentionMetadataBuilder.get_persistent_workspace_memory_profiling_support()returnsREQUIRED,NEUTRALorUNSUPPORTED, and defaults toUNSUPPORTED. The new lifecycle runs only when every builder of the model declaresREQUIREDorNEUTRALand at least one declaresREQUIRED.NEUTRALmeans a builder has no reservation of its own but can be carried by the common lease; a builder that has not been taught the contract keeps the default and the model stays off the path. FlashInfer declaresREQUIRED; GDN declaresNEUTRALso a hybrid model can still take it.Lease lifetime. The shared arena is owned by the global workspace manager, but the int workspace is owned by its wrapper alone. Releasing the lease inside the profiling block frees it before the closing measurement
total_consumedis derived from, so the lease has to outlive that block. The order is: initial profiling snapshot, persistent workspace reservation and profile run, closing profiling measurement, lease release, CUDA graph profiling, then rebind, warmup and lock as driven by the runner lifecycle.Conservative reservation. This PR does not query per-plan workspace sizes. FlashInfer reserves its existing default arena and materializes the wrappers the enabled prefill and decode routes will use, while memory profiling is measuring allocator use, so the int workspace each one allocates falls inside the measured window. Direct trtllm-gen's module-global workspace is materialized the same way.
A model that does not opt in does not enter the new profiling-time reservation or post-warmup lock path; the existing capture-time behavior is unchanged.
Accounting for that persistent allocation can reduce the available KV capacity. The actual change has not been measured on this branch.
Test Plan
Static checks:
py_compileon the 12 changed Python filesruff checkruff format --checkgit diff --checkpre-commit run mypy-3.10Focused test:
Test Result
Static checks passed. The focused run is
69 passed, 0 failed, 0 skipped.It covers the support tri-state and its fail-closed default,
REQUIRED+NEUTRALcomposition on a mixed FlashInfer/GDN model, and the lease keeping a builder's int workspace alive through the closing measurement and released before CUDA graph profiling. It also covers the V1 and V2 ordering around that measurement, per-ubatch arena isolation, and arena size and pointer stability after the lock.Notes
This PR deliberately leaves mm-prefix on the unsupported profiling path. It fixes the shared lifecycle for builders that opt in; mm-prefix will need a separate integration that materializes and retains its dedicated wrapper before it can safely opt in.
The results above come from CPU-only tests, against stubs and real
WorkspaceManagerinstances. Not exercised on this branch:UNSUPPORTEDand is covered statically onlytests/v1/worker/Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Not needed; this is an internal FlashInfer workspace accounting change.