#920 with renames#943
Conversation
…o ben/make-v3-default
Switch ModelSource.get_{classifier,regressor}_v3() default_filename
and filenames from the dated form (tabpfn-v3-{role}-20260506.ckpt)
to the canonical wire-format name used everywhere else in the stack
(tabpfn-v3-{role}-v3_default.ckpt). This is the form produced by
tabpfn-client's `_model_name_to_path("v3_default")` and pinned by
tabpfn-server's STARTUP_MODELS, so a single source of truth flows
end-to-end with no rename layer in between.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the default model version to V3, integrates V3 support across architectures and tests, and refactors architecture name resolution. Feedback identifies a potential TypeError in save_tabpfn_model from redundant configuration handling and suggests a more precise prefix-based filter for state dict keys in load_model to avoid stripping valid parameters.
| "architecture_name": architecture_name, | ||
| "inference_config": asdict(model.inference_config_), |
There was a problem hiding this comment.
This added line is redundant and potentially problematic. The inference_config is already retrieved using getattr(model, "inference_config_", None) at line 1083 and added to the checkpoint dict conditionally at lines 1108-1109. Including it here unconditionally will cause a TypeError if model.inference_config_ is None (as asdict(None) is invalid). It is better to rely on the existing conditional block below.
| "architecture_name": architecture_name, | |
| "inference_config": asdict(model.inference_config_), | |
| "architecture_name": architecture_name, |
| model.load_state_dict(full_state) | ||
| # The model computes the loss internally. Strip criterion keys that | ||
| # save_tabpfn_model may have written so load_state_dict doesn't reject them. | ||
| model_state = {k: v for k, v in full_state.items() if "criterion." not in k} |
There was a problem hiding this comment.
The filtering condition "criterion." not in k is too broad and might accidentally strip valid model parameters if their names contain the string "criterion." (e.g., layer.criterion_weight). Since save_tabpfn_model specifically prefixes criterion keys with criterion., it is safer and more precise to use not k.startswith("criterion.").
| model_state = {k: v for k, v in full_state.items() if "criterion." not in k} | |
| model_state = {k: v for k, v in full_state.items() if not k.startswith("criterion.")} |
|
ah just saw your comment on the other pr and realised this is probably an auto-assign! |
|
Sorry about that! Was indeed an auto-assign. We won't merge this as-is. Keeping it open for now until we've decided how to untangle our pins (not strictly necessary -- just a convenient bookmark) |
3 participants
This is just @bejaeger's #920, but with renames to be compatible with the client package