Repository navigation
Conversation
|
My proposal to make RASR use the GPU state manager automatically whenever the session runs with the CUDA execution provider, such that setting
What do you think about that? @SimBe195 @curufinwe |
| } | ||
| } | ||
|
|
||
| std::vector<float const*> StateCopier::deviceData(std::vector<Value> const& tensors) { |
There was a problem hiding this comment.
The upload of host tensors into stagingBuffer_ never runs because mergeStates always feeds device tensors and both users (StateManagedOnnxLabelScorer and OnnxRecurrentLanguageModel) run the session with default output locations. So Session::run already puts the state outputs on the device. Maybe remove the staging code and stagingBuffer_ and instead just require that the tensors are on device.
There was a problem hiding this comment.
You're right that at the moment the staging code is never used and I will therefore remove it. However, the case does occur for label scorers with an initializer model that has no state inputs, e.g. the prefix LM prefill in the PrefixSpeechLmOnnxLabelScorer. We will need to run the initializer there with device output locations for the state outputs when the state manager keeps its states on device.
| } | ||
|
|
||
| // Abort with a message if `status` (a cudaError_t) is an error | ||
| void checkCudaStatus(int status, char const* what); |
There was a problem hiding this comment.
This header is anyway only compiled with MODULE_CUDA active, so you can include <cuda_runtime.h> and use cudaError_t directly. This also allows getting rid of the checkCuda forwarding wrapper in DeviceVector.cc and the casts.
| * For each task copy `stepSize` floats between block and tensor. Block element i is tensor element | ||
| * `blockOffsets[i / blockSize] + i % blockSize`. `tasks`/`blockOffsets` are device pointers, returns a cudaError_t. | ||
| */ | ||
| int launchBlockCopy(BlockCopyTask const* tasks, |
There was a problem hiding this comment.
Here you could also include <cuda_runtime.h> and use cudaError_t directly as return type instead of an int (similar as checkCudaStatus in DeviceVector.hh).
| size_t blockSize, | ||
| size_t stepSize, | ||
| BlockCopyDirection direction, | ||
| void* stream); |
There was a problem hiding this comment.
This can be a cudaStream_t instead of void*. For Session::sharedCudaStream() it makes sense to return void* since that must also be compilable without MODULE_CUDA but the cast back to cudaStream_t is repeated in three local sharedStream helper functions (DeviceVector.cc:33, StateCopier.cc:28 and TransformerGpuStateManger.cc:87). This could be unified via one inline sharedStream helper in DeviceVector.hh.
| if (not alwaysIncludeFirstTokenState_ or p != 0ul) { | ||
| idx += prefix_offset + p; | ||
| } | ||
| auto const* state = dynamic_cast<DeviceVector const*>(prefix_states[idx]->at(v).get()); |
There was a problem hiding this comment.
This dynamic cast runs in the innermost loop which can come out to hundreds of thousands of casts per step with large history or batch size. Prefer static_cast instead. If you want the check, maybe do it only on the first state.
| return cudaSuccess; | ||
| } | ||
| bool const toTensor = direction == BlockCopyDirection::BlockToTensor; | ||
| for (size_t start = 0ul; start < numTasks; start += kMaxGridSize) { |
There was a problem hiding this comment.
This loop is never going to run multiple iterations. That would mean that a single variable has over 2^30 tasks which is unrealistic. You can just get rid of the loop and do a single launch.
|
|
||
| float DeviceVector::get(size_t pos) const { | ||
| require_lt(pos, size_); | ||
| return toHost()[pos]; |
There was a problem hiding this comment.
This copies the full vector to host just to read one float. Do a single-element cudaMemcpy from data_ + pos instead.
| StateVariables const& vars, | ||
| std::vector<size_t>& suffix_lengths, | ||
| std::vector<Value> const& state_tensors, | ||
| Nn::CompressedVectorFactory<float> const& vector_factory) { |
There was a problem hiding this comment.
vector_factory is not used, so you could leave it unnamed here (like in initialState).
| auto& tasks = batches.back().tasks; | ||
| float const* base = bases[v]; | ||
|
|
||
| size_t const timeAxis = timeAxisOf(vars[v]); |
There was a problem hiding this comment.
timeAxisOf(vars[v]) is computed twice per variable (first time in line 233). It could be saved in StepLayout instead.
| #ifdef MODULE_CUDA | ||
| return std::unique_ptr<AbstractStateManager<Onnx::Value, Onnx::OnnxStateVariable>>( | ||
| new Onnx::TransformerGpuStateManager(config)); | ||
| #else | ||
| Core::Application::us()->criticalError("State manager type transformer-gpu requires RASR compiled with MODULE_CUDA"); | ||
| return nullptr; | ||
| #endif |
There was a problem hiding this comment.
Instead of the criticalError + nullptr return here, you can conditionally only register "transformer-gpu" if MODULE_CUDA is enabled. Then the value is rejected by the choice parser already. But this anyway should go away when we adopt the backend auto-selection.
|
The code in this PR contains a wild mix of snake_case and camelCase naming, probably because some parts were copied from the CPU state manager and other parts written from scratch. It would be better to make it consistently camelCase since that's the usual convention in RASR. Also, there are several usages of the term "block" in different contexts which mean different things. It can be a pool block, a contiguous span of data in a tensor or a CUDA thread block. This can make it confusing to follow. I'd prefer some more distinctive names. |
| HistoryState result; | ||
| result.reserve(vars.size()); | ||
| for (size_t v = 0ul; v < vars.size(); ++v) { | ||
| result.emplace_back(new DeviceVector(nullptr, 0ul)); |
There was a problem hiding this comment.
Please prefer smart pointers instead of raw new. Same thing also further down on line 268.
|
In |
I like the approach of having a getter function in I would also keep explicit choice values which force the state manager For example one plain "transformer" which auto-selects and "transformer-cpu"/"transformer-cuda" which force a selection. And make sure that "-16bit" and "-8bit" variants don't switch since they are not currently supported on GPU. In fact, maybe a warning should be logged if |
This PR adds a transformer state manager that keeps the states, e.g. the KV cache, in GPU memory. It behaves like the
transformerstate manager, including the parametersmax-historyandalways-include-first-token-state.The search, label scorers and LMs are unchanged.Each state is stored as a
DeviceVector, aCompressedVectorwhose data lives in a block of a GPU memory pool. This keeps theHistoryStateinterface unchanged.mergeStatesassembles the batched state inputs with a small CUDA kernel and passes them to the session as device tensors. With #281, the session then keeps the state outputs on the GPU, andsplitStatescopies them into new pool blocks. Outputs that arrive in host memory, e.g. from a model without state inputs, are uploaded first. This removes the host-side merge and split and the copies of all states between host and GPU in every search step.The pool only frees its memory when the state manager is destroyed, freed blocks are reused. GPU memory therefore follows the peak number of states that are alive at the same time. For label scorers, this is bounded by their caches (
max-cached-score-vectors) and for theOnnxRecurrentLanguageModelby the number of live LM histories (as with thetransformerstate manager in host memory).Depends on #281.