Fix vocab kwarg collision in eval_testdata's PertBatchCollator call - #53
Conversation
PertTFModel.__init__ set _hub_mixin_config["vocab"] = None instead of deleting the key, so every model's training_config carried vocab: None. eval_testdata unpacked that config into PertBatchCollator(vocab, ..., **collator_config), raising "got multiple values for argument 'vocab'". - Delete the vocab key from the hub config in __init__ (ntoken is already captured and model.vocab is stored separately; nothing reads training_config.vocab). - Strip vocab/gene_ids/hvg_inds/full_tokenize from collator_config in eval_testdata so checkpoints saved with "vocab": null keep working. Fixes #52 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhowxpBYw4dLuu1rBnRx9s
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c91a66c8b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for key in ("vocab", "gene_ids", "hvg_inds", "full_tokenize"): | ||
| collator_config.pop(key, None) |
There was a problem hiding this comment.
When callers provide full_tokenize=True in config, removing this key silently changes evaluation to the collator's default False. Unlike the other keys here, full_tokenize does not collide with an explicitly supplied argument at the call below; dropping it causes evaluation to use max_seq_len and potentially omit genes instead of processing the full sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 51fbdc5: the loop now strips only vocab, gene_ids, and hvg_inds, which the call supplies explicitly. full_tokenize is left in the config.
Only strip the keys the collator call supplies explicitly (vocab, gene_ids, hvg_inds). full_tokenize is not passed by eval_testdata, so removing it silently changed a caller's full-token request to the collator default. Addresses Codex review on #53. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhowxpBYw4dLuu1rBnRx9s
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Fixes #52.
eval_testdataraisedTypeError: PertBatchCollator.__init__() got multiple values for argument 'vocab'whenever it was called with a config derived frommodel.training_config.Root cause
HFPerturbationTFModel.__init__set_hub_mixin_config["vocab"] = Noneinstead of deleting the key, then copied_hub_mixin_configintoself.training_config. Every model therefore carriedvocab: Nonein its training config, and the value round-tripped throughtraining_config.json.eval_testdatadidcollator_config = dict(config)and calledPertBatchCollator(vocab, gene_ids, hvg_inds=hvg_inds, **collator_config), passingvocabboth positionally and as a keyword.finetunealready deletedvocabfrom its config for this reason;eval_testdatanever got the same guard.Changes
perttf/model/hf.py: delete thevocabkey from the hub config in__init__rather than setting it toNone.ntokenis captured on the preceding line,model.vocabis stored separately, and nothing in the package readstraining_config.vocab.perttf/model/train_function.py: stripvocab,gene_ids,hvg_inds, andfull_tokenizefromcollator_configineval_testdata, so checkpoints already saved with"vocab": nullkeep working.Testing
python -m py_compile.🤖 Generated with Claude Code