Skip to content

Fix maximum value check for token_batch_size to allow larger values#130

Merged
luciaquirke merged 1 commit intoEleutherAI:mainfrom
smarter:tbs
Feb 9, 2026
Merged

Fix maximum value check for token_batch_size to allow larger values#130
luciaquirke merged 1 commit intoEleutherAI:mainfrom
smarter:tbs

Conversation

@smarter
Copy link
Collaborator

@smarter smarter commented Jan 22, 2026

Previously, token_batch_size was limited by max_position_embeddings, but this is the maximum length of one sequence, and a batch can contain multiple sequences. As far as I can tell, the real limit is the default value of model_max_length in the tokenizer (some models have model_max_length == max_position_embeddings which causes this confusion).

We also make the truncation logic more explicit by passing a max_length parameter instead of mutating model_max_len.

@smarter smarter force-pushed the tbs branch 5 times, most recently from f677706 to c9f4fef Compare January 22, 2026 19:10
@LouisYRYJ LouisYRYJ requested a review from luciaquirke January 29, 2026 14:19
@luciaquirke
Copy link
Collaborator

luciaquirke commented Jan 30, 2026

LGTM + thanks very much for the fix! let's merge when conflicts are resolved

@luciaquirke
Copy link
Collaborator

@claude could you please fix the merge conflicts

@claude
Copy link

claude bot commented Feb 3, 2026

Claude encountered an error —— View job

Command failed: git fetch origin --depth=20 tbs

I'll analyze this and get back to you.

Previously, token_batch_size was limited by max_position_embeddings, but this is
the maximum length of one sequence, and a batch can contain multiple sequences.
As far as I can tell, the real limit is the default value of model_max_length in
the tokenizer (some models have model_max_length == max_position_embeddings
which causes this confusion).

We also make the truncation logic more explicit by passing a max_length
parameter instead of mutating model_max_len.
@smarter
Copy link
Collaborator Author

smarter commented Feb 7, 2026

Rebased.

@smarter smarter requested a review from luciaquirke February 7, 2026 15:26
@luciaquirke luciaquirke merged commit 72975c9 into EleutherAI:main Feb 9, 2026
5 of 6 checks passed
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