Skip to content

Add tool-schema support to SFT tokenization#1746

Open
hamishivi wants to merge 16 commits into
allenai:mainfrom
hamishivi:remake/pr-1734-sft-tools-support
Open

Add tool-schema support to SFT tokenization#1746
hamishivi wants to merge 16 commits into
allenai:mainfrom
hamishivi:remake/pr-1734-sft-tools-support

Conversation

@hamishivi

Copy link
Copy Markdown
Contributor

Replacement for #1734, rebased onto current main and pushed from hamishivi/open-instruct.

Summary

  • Pass the dataset tools column to apply_chat_template during SFT tokenization so tool schemas are rendered into the prompt. A new _normalize_tools_for_chat_template helper accepts a list/dict/JSON-string (and treats empty/None as no tools), rejecting tool-name-string lists.
  • Derive assistant labels from token offset mappings against the rendered conversation (prefix-stable per-assistant-turn spans) instead of mask_labels, so labels stay correct when tools are present.
  • Consume the tools column during SFT tokenization rather than persisting it to the cached dataset (_SFT_TOKENIZE_FNS).
  • Add sft_tulu_tokenize_without_truncation_v1 and register last_turn_tulu_tokenize_and_truncate_v1 (which now also forwards tools).
  • Bump DATASET_CACHE_VERSION to v7 to invalidate stale caches.

Notes

  • For the no-tools tulu path the new tokenizer is byte-identical to the previous mask_labels path: the existing GOLD_SFT golden-hash test passes unchanged.
  • This PR intentionally excludes the unrelated dataset_config_name plumbing and the sft_filter_v1 rename that were bundled alongside this work on the source branch.

Test plan

  • uv run pytest open_instruct/test_dataset_transformation.py (20 passed locally), including:
    • existing GOLD_SFT/preference/rlvr golden-hash tests (unchanged),
    • new TestToolNormalization (JSON parsing, dict-wrapping, empty handling, rejection cases),
    • new TestSFTTuluTokenizeLabels (assistant-only trainable labels, tools column consumed, no-truncation variant).

GPU_TESTS=bypass

Made with Cursor

hamishivi and others added 16 commits June 27, 2026 18:54
Pass the dataset `tools` column to `apply_chat_template` during SFT
tokenization so tool schemas are rendered into the prompt. JSON-encoded tool
schemas are parsed via a new `_normalize_tools_for_chat_template` helper.

Assistant labels are now derived from token offset mappings against the
rendered conversation (prefix-stable per-assistant-turn spans), which keeps
labels correct when tools are present. The tools column is consumed by SFT
tokenization rather than persisted to the cached dataset. Bumps the dataset
cache version to v7. Adds `sft_tulu_tokenize_without_truncation_v1` and
registers `last_turn_tulu_tokenize_and_truncate_v1`.

Co-authored-by: Cursor <cursoragent@cursor.com>
A JSON-encoded "null" (or "") decodes to None/empty after json.loads, which
previously fell through to the list check and raised a spurious TypeError.
Re-check for None/empty after parsing so these decode to no tools gracefully.

Co-authored-by: Cursor <cursoragent@cursor.com>
The offset-mapping-based label derivation relies on return_offsets_mapping,
which slow (Python) tokenizers do not support. Raise a clear error pointing at
use_fast=True instead of the opaque ValueError/NotImplementedError.

Co-authored-by: Cursor <cursoragent@cursor.com>
…heuristic

The previous content_offset = assistant_text.find(chr(10)) heuristic assumed the
assistant header always ends in a newline and the content never starts with one,
which mis-masks templates like simple_chat (no newline header) or
simple_concat_with_space (no header, multiline content). Locate the actual content
string (rfind, falling back to the newline heuristic) and train any token that
overlaps the content span so a header/content boundary token stays trainable. The
tulu template result (and its golden-hash test) is unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
Type the normalized tools as a gradual list (assignable to apply_chat_template's
list[dict | Callable]) and assert the tokenize=False renderings are str so ty can
narrow them before startswith/slicing.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add a last_turn_only flag to _tokenize_tulu_sft_with_assistant_labels and route
last_turn_tulu_tokenize_and_truncate_v1 through it, so the last-turn path forwards
the tools column to the chat template and drops the legacy mask_labels path. Also
guard the content match with content.strip() to avoid matching whitespace-only
content. Golden preference/SFT hashes are unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
pandas/CSV-backed datasets can represent a missing object cell as float('nan');
normalize it to no tools instead of falling through to a TypeError.

Co-authored-by: Cursor <cursoragent@cursor.com>
Search for content.strip() in the rendered text so leading/trailing whitespace
in the message content (which templates may strip) doesn't make rfind miss and
fall back to the newline heuristic.

Co-authored-by: Cursor <cursoragent@cursor.com>
Find the index of the last assistant message instead of assuming it is the final
message, so a conversation ending with a user/system turn still trains its real
last assistant turn.

Co-authored-by: Cursor <cursoragent@cursor.com>
Some chat templates append eos_token only on the final turn (loop.last), so
rendered_before ends with an eos that is absent at that position in the longer
render, spuriously failing the prefix-stability check. Strip a trailing eos_token
and retry before raising.

Co-authored-by: Cursor <cursoragent@cursor.com>
When the first message is an assistant turn (message_idx == 0), messages[:0] is
empty and apply_chat_template([]) raises in most tokenizers; treat the prefix as
an empty string instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
When target_columns is None it defaults to dataset.column_names (which includes
tools), so skipping _preserve_column was insufficient — explicitly remove
TOOLS_COLUMN_KEY for SFT tokenization so the consumed column isn't persisted.

Co-authored-by: Cursor <cursoragent@cursor.com>
When the assistant content can't be located verbatim, derive the header offset
from add_generation_prompt (the exact assistant header the template emits) and
raise if it can't be determined, replacing the fragile first-newline heuristic
that mis-masked header-without-newline templates with multiline content.

Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the accumulated rfind/eos-strip/newline/generation-prompt-fallback logic
with one rule: the trainable span is [len(render(messages[:k], add_generation_prompt=True)),
len(render(messages[:k+1]))], with both required to be prefixes of the full render.
If not (e.g. templates that append eos only on the final turn, or lack
add_generation_prompt support), raise a clear error instead of silently mis-masking.
Tulu output (and the golden hashes) are unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces tool-schema support to SFT tokenization by parsing the tools column, passing it to apply_chat_template, and deriving assistant labels from offset mappings using fast tokenizers. It also ensures the tools column is consumed rather than persisted, and adds comprehensive unit tests. The reviewer feedback suggests two valuable improvements: stripping whitespace from the tools string before parsing to handle whitespace-only strings safely, and adding an explicit check to verify that add_generation_prompt actually appends the assistant header to prevent silent mis-masking when the template does not support it.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +957 to +964
if isinstance(tools, str):
try:
tools = json.loads(tools)
except json.JSONDecodeError as exc:
raise ValueError(f"{TOOLS_COLUMN_KEY} must be a JSON-encoded tool schema list, got: {tools!r}") from exc
# Re-check after parsing: a JSON "null" or "" decodes to None / "".
if tools is None or tools == "":
return None

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.

medium

When parsing the tools column from a string, it is safer to strip any leading or trailing whitespace first. This prevents potential parsing issues with whitespace-only strings (e.g., from CSV/TSV exports) and allows explicitly treating them as empty/None.

Suggested change
if isinstance(tools, str):
try:
tools = json.loads(tools)
except json.JSONDecodeError as exc:
raise ValueError(f"{TOOLS_COLUMN_KEY} must be a JSON-encoded tool schema list, got: {tools!r}") from exc
# Re-check after parsing: a JSON "null" or "" decodes to None / "".
if tools is None or tools == "":
return None
if isinstance(tools, str):
tools_str = tools.strip()
if not tools_str or tools_str.lower() == "null":
return None
try:
tools = json.loads(tools_str)
except json.JSONDecodeError as exc:
raise ValueError(f"{TOOLS_COLUMN_KEY} must be a JSON-encoded tool schema list, got: {tools!r}") from exc
# Re-check after parsing: a JSON "null" or "" decodes to None / "".
if tools is None or tools == "":
return None

Comment on lines +1259 to +1267
if message_idx == 0:
header = ""
else:
header = tokenizer.apply_chat_template(
conversation=messages[:message_idx], tools=tools, tokenize=False, add_generation_prompt=True
)
through = tokenizer.apply_chat_template(
conversation=messages[: message_idx + 1], tools=tools, tokenize=False, add_generation_prompt=False
)

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.

medium

If the chat template does not support add_generation_prompt (i.e., it silently ignores the argument), header (rendered with add_generation_prompt=True) will be identical to the prefix rendered with add_generation_prompt=False. In this case, the prefix-stability check won't fail, but the assistant header will be silently trained because the trainable span will start before the assistant content. Adding an explicit check to ensure add_generation_prompt actually appended the assistant header prevents this silent mis-masking.

Suggested change
if message_idx == 0:
header = ""
else:
header = tokenizer.apply_chat_template(
conversation=messages[:message_idx], tools=tools, tokenize=False, add_generation_prompt=True
)
through = tokenizer.apply_chat_template(
conversation=messages[: message_idx + 1], tools=tools, tokenize=False, add_generation_prompt=False
)
if message_idx == 0:
header = ""
else:
header = tokenizer.apply_chat_template(
conversation=messages[:message_idx], tools=tools, tokenize=False, add_generation_prompt=True
)
# Ensure add_generation_prompt actually appended the assistant header to avoid silent mis-masking
header_without_prompt = tokenizer.apply_chat_template(
conversation=messages[:message_idx], tools=tools, tokenize=False, add_generation_prompt=False
)
if header == header_without_prompt:
raise ValueError(
"Cannot compute assistant label spans: the chat template does not support "
"add_generation_prompt or failed to append the assistant header."
)
through = tokenizer.apply_chat_template(
conversation=messages[: message_idx + 1], tools=tools, tokenize=False, add_generation_prompt=False
)

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.

1 participant