directly pass the model path through + simplify the v2 vs v2.5 logic#177
directly pass the model path through + simplify the v2 vs v2.5 logic#177
Conversation
Summary of ChangesHello @LeoGrin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the model path handling and selection logic within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request does a great job of simplifying the model selection logic by removing the TabPFNModelSelection class and passing the model_path directly. The new default of "auto" is a sensible change that improves robustness. The associated test removals are also appropriate.
I have a few suggestions to further improve the code:
- I've pointed out some code duplication in
create_default_for_versionbetween the classifier and regressor that could be refactored into a base class to improve maintainability. - I've also suggested re-adding simplified tests to verify that a custom
model_pathis correctly passed to the inference client, as removing the old tests has created a small gap in test coverage for this functionality.
Overall, these are solid changes that make the client easier to understand and use.
brendan-priorlabs
left a comment
There was a problem hiding this comment.
Unfortunately this looks like a breaking change. I don't think we can go ahead with it easily. Note that tabpfn-time-series and tabpfn-extensions both have dependencies on our existing setup as do users. Let's discuss more in person?
Change Description
Try to be precise. You can additionally add comments to your PR, this might help the reviewer a lot.
Pass the model path directly instead of having a different format than the TabPFN package, which simplify the logic. Tried
quick_test.pywhich is working except for the default checkpoint which is not on the server yet (https://github.com/PriorLabs/tabpfn-server/pull/432).If you used new dependencies: Did you add them to
requirements.txt?Who did you ping on Mattermost to review your PR? Please ping that person again whenever you are ready for another review.
Breaking changes
If you made any breaking changes, please update the version number.
Breaking changes are totally fine, we just need to make sure to keep the users informed and the server in sync.
Does this PR break the API? If so, what is the corresponding server commit?
Does this PR break the user interface? If so, why?
Please do not mark comments/conversations as resolved unless you are the assigned reviewer. This helps maintain clarity during the review process.
Note
Directly pass
model_paththrough and replace name-to-path selection with simple version defaults for v2/v2.5 across classifier and regressor, updating tests accordingly.tabpfn_client/estimator.py):TabPFNModelSelection), name-to-path mapping, and related constants._default_model_path_v2()and_default_model_path_v2_5(); use explicit v2 ckpt paths andNonefor v2.5 auto-selection.TabPFNClassifierandTabPFNRegressor:model_path: Optional[str] = None.create_default_for_version()to setmodel_pathvia helpers.fit/predictpassself.model_pathdirectly inconfig._model_name_to_path.model_pathis forwarded to server config andcreate_default_for_version()sets v2 defaults,Nonefor v2.5, applies overrides, and errors on invalid version.Written by Cursor Bugbot for commit fb80d35. This will update automatically on new commits. Configure here.