Skip to content

Refactor and fixes - #73

Open
ngc92 wants to merge 6 commits into
devfrom
refactor
Open

Refactor and fixes#73
ngc92 wants to merge 6 commits into
devfrom
refactor

Conversation

@ngc92

@ngc92 ngc92 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

some more refactoring, adding support for mistral (llama-like) architectures, fix matmul for larger shapes.

ngc92 and others added 4 commits July 19, 2026 16:57
The index -> HF-name mapping existed in three near-identical copies
(sLLamaWeights::iterate_tensors, OptStateWrapper::iterate_tensors, and
MultiGPUPyTrainer::get_gradients). Replace them with name lookup
functions in LLamaWeightID and index loops that skip empty tensors.
This also makes get_gradients independent of the concrete weight layout,
resolving its TODO.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
It only reads the data pointer and dtype of the source; narrow the
parameter to Tensor so that the remaining TensorShard-taking interfaces
are exactly the ones that consume shard metadata (safetensors IO via
iterate_tensors).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The array is byte storage reinterpreted as x128; declaring it as char*
gave it pointer element type and only 8-byte guaranteed alignment.
(Spotted via the surogate fork, which hit real misalignment here.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The trainer's batch_size is the per-GPU micro-batch while step() takes
the global batch; the harnesses passed the global batch size straight
through, so any multi-GPU run failed the shape check. Divide by the GPU
count instead, keeping the total batch (and thus gradients) independent
of the GPU count. Gather the per-rank gradient shards in torch_reference
so the comparison reconstructs full tensors, and add a pytest suite
running the 2-GPU parity matrix (data-parallel, shard-weights,
shard-gradients, both).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 14:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors model/weight naming utilities, expands architecture support to include Mistral (LLaMA-like), hardens config loading against unsupported HF settings, and fixes GEMM address calculations for very large matrix shapes. It also adds integration tests to validate gradient parity across architectures and across 2-GPU training modes.

Changes:

  • Add gradient-parity integration tests for supported/refused architectures and for 2-GPU runs with different sharding modes.
  • Add Mistral architecture handling in transformer config load/save, plus explicit rejection of unsupported config features (rope_scaling, attention_bias, etc.).
  • Refactor weight/gradient naming to use centralized name helpers; update GEMM kernel indexing to use wider integer arithmetic and add a large-output regression test.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/test_multi_gpu.py Adds end-to-end 2-GPU gradient parity test runner wrapper.
test/test_architectures.py Adds per-architecture gradient parity + “must refuse” negative fixtures.
src/utilities/comm.h Adjusts NCCL all-gather API to accept Tensor instead of TensorShard.
src/utilities/comm.cpp Updates NCCL all-gather implementation signature accordingly.
src/training/transformer_config.h Adds MISTRAL architecture enum.
src/training/transformer_config.cpp Adds Mistral load/save support and explicit rejection of unsupported HF config fields.
src/testing/test-gemm.cpp Adds large-output regression test for GEMM INT overflow bug.
src/models/llama_weights.h Adds centralized HF weight-name helpers and a typed accessor for block tensors.
src/models/llama_weights.cpp Refactors tensor iteration/naming to use centralized name helpers.
src/models/llama_optimizer.cpp Refactors optimizer-state tensor naming/iteration to use centralized name helpers.
src/kernels/rmsnorm.cu Fixes shared-memory declaration/alignment for RMSNorm kernels.
src/kernels/gemm_mma.cu Uses wider index arithmetic to avoid overflow for large m*k / m*n.
src/binding/python/tests/torch_reference.py Fixes multi-GPU gradient retrieval semantics (per-GPU shard concat) and per-GPU microbatch sizing.
src/binding/python/tests/run.py Makes training test runner respect config.gpus and divides global batch into per-GPU microbatches.
src/binding/py_train.cpp Generalizes multi-GPU gradient enumeration via centralized weight-name helpers.
scripts/create_tiny_test_model.py Adds script to generate tiny HF-cache fixtures for integration tests (incl. mistral).
Comments suppressed due to low confidence (2)

src/testing/test-gemm.cpp:289

  • cudaMemset sizes for a and b are also missing sizeof(__nv_fp8_e4m3), so only a fraction of each buffer is initialized (and if the allocations are fixed, these would become short writes).
    CUDA_CHECK(cudaMemset(a, 0x38, (std::size_t)m * k));
    CUDA_CHECK(cudaMemset(b, 0x38, (std::size_t)n * k));

src/kernels/gemm_mma.cu:217

  • out_offset is intended to be 64-bit (per the comment), but it's declared as long, which may be 32-bit depending on platform/toolchain. Use an explicitly 64-bit type for out_offset so the INT overflow fix is robust.
            // 64 bit: (row index) * n overflows int once m * n exceeds 2^31
            long out_offset = ((long)(i + ii) * TI + r) * n + (j + jj) * TJ + 8 * c;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/testing/test-gemm.cpp
Comment thread src/training/transformer_config.cpp
Comment thread src/kernels/gemm_mma.cu
ngc92 and others added 2 commits July 23, 2026 16:53
Mistral is the only architecture string among the llama-compatible models
that we did not accept yet; everything else in that group already declares
LlamaForCausalLM or Qwen2ForCausalLM and worked unchanged.

More importantly, several config.json fields were inferred from the
architecture enum or ignored outright, so a checkpoint could be loaded as a
model that differs from what it describes -- rope_scaling (Llama-3.1,
deepseek-coder) was silently dropped, which only shows up as degraded
quality. Read those fields now and refuse what we cannot express exactly:
rope_scaling, mlp_bias, attention_bias, a non-silu hidden_act, non-zero
attention_dropout, and an active sliding window. Note that Qwen ships an
inactive sliding_window that must keep loading.

HF's attention_bias also biases o_proj, for which we have no tensor, so it
is rejected rather than mapped onto UseQKVBias, which means q/k/v only. The
same asymmetry applies when writing: a q/k/v-only bias has no faithful
representation in the llama schema, since true would promise an o_proj bias
we lack and false would disclaim biases we have. Saving such a config is
therefore refused too, so we cannot emit a config.json we would then reject.

scripts/create_tiny_test_model.py grows the architectures it can emit, and
test/test_architectures.py compares every parameter gradient against
transformers for each of them, plus checks that the unrepresentable configs
are refused with a decent message. test-transformer-config.cpp covers the
save/load round trip per architecture, which is what pins down the
save-versus-load asymmetry above.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The epilogue computed the output address as `((i + ii) * TI + r) * n` in `int`, so once
m * n exceeds 2^31 the product wraps and the kernel writes to a bogus address -- an illegal
memory access rather than a wrong result. An LM head of 16384 x 151936 is already past the
limit, which is why this showed up on an L40 and not on the 16 GB cards here. The A/B row
offsets get the same widening, where the product is m * k / 16.

Confirmed both ways: m*n = 2.145e9 runs clean, 2.416e9 faults, and compute-sanitizer pins it
to the 16-byte store in the epilogue. All three epilogue paths share the index. The added
regression test allocates a >INT_MAX output (~5 GB, skipped if it does not fit) and was
verified to fail without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comment thread src/models/llama_weights.h
Comment thread src/utilities/comm.cpp
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.

2 participants