Skip to content

fixed needle rope bug, moved repeated rope table instantiations from … - #713

Merged
HenryNdubuaku merged 1 commit into
mainfrom
karen/rope_disk_optimization
Jun 11, 2026
Merged

fixed needle rope bug, moved repeated rope table instantiations from …#713
HenryNdubuaku merged 1 commit into
mainfrom
karen/rope_disk_optimization

Conversation

@kar-m

@kar-m kar-m commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

…components into weights

Copilot AI review requested due to automatic review settings June 11, 2026 03:32
@kar-m

kar-m commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

@jakmro if you look at current model bundles for qwen, lfm, and gemma, they are pretty big outside of the weights because rope tables were being stored in the component folders. Some of these tables were being stored in several copies (gemma being the worst offender, storing 1.2gb worth of rope tables). Changed it so unique rope tables are stored in weights and just reused between components

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 targets Needle long-context / RoPE stability and reduces duplicated RoPE table materialization across component graphs by externalizing shared tables into weight files, plus adds an engine-level Needle tool-calling test.

Changes:

  • Update RoPE table precompute matching logic in optimize_graph.py (remove constant-ancestor tracing for inv_freq).
  • Pad Needle input_ids up to a fixed context length (1024) when building component specs.
  • Deduplicate/externalize c_rope_table_* bound constants into shared .weights files during bundle writing, and add a new Needle tool-call engine test.

Reviewed changes

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

File Description
python/cactus/transpile/optimize_graph.py Adjusts how RoPE precompute identifies the inv_freq constant input.
python/cactus/transpile/model_adapters.py Pads Needle inputs to a fixed context length and builds attention masks.
python/cactus/transpile/hf_model.py Externalizes/deduplicates shared RoPE tables into standalone weight files during bundle creation.
cactus-engine/tests/test_needle.cpp Adds a Needle-specific engine regression test for forced tool calling.
Comments suppressed due to low confidence (1)

python/cactus/transpile/model_adapters.py:5342

  • When pad_token_id is None and input_ids are padded up to NEEDLE_CONTEXT_LENGTH, the code sets attention_mask = torch.ones_like(...), which marks the newly padded tokens as real tokens. This changes the encoder inputs vs. the original shorter sequence and can break Needle models that rely on masking rather than a dedicated pad token. Build the mask using current_len so padded positions are 0 even when pad_token_id is unknown.
    if pad_token_id is None:
        attention_mask = torch.ones_like(input_ids, dtype=torch.int64)
    else:
        attention_mask = (input_ids != int(pad_token_id)).to(dtype=torch.int64)

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

Comment on lines +77 to +78
const bool has_function_calls =
r.find("\"function_calls\":[{") != std::string::npos;
Comment on lines 379 to +381
manifest_components: list[dict[str, object]] = []

rope_table_registry: dict[str, str] = {}
@HenryNdubuaku
HenryNdubuaku merged commit 5c17824 into main Jun 11, 2026
4 of 6 checks passed
ncylich added a commit that referenced this pull request Jun 11, 2026
Union resolutions: rope-table externalization (#713) runs before the
general materialized-constant externalizer; decoder_embed_chunk (#714)
emitted via the unified LFM2 chunked-spec helper and added to the qwen3
text component plan; gemma3 and needle convert families coexist.

Signed-off-by: Noah Cylich <noahcylich@gmail.com>
ncylich added a commit that referenced this pull request Jun 11, 2026
Union resolutions: rope-table externalization (#713) runs before the
general materialized-constant externalizer; decoder_embed_chunk (#714)
emitted via the unified LFM2 chunked-spec helper and added to the qwen3
text component plan; gemma3 and needle convert families coexist.

Signed-off-by: Noah Cylich <noahcylich@gmail.com>
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.

3 participants