Load the inference config from the checkpoint, if present. #576
Conversation
|
/gemini review |
![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 refactors the handling of InferenceConfig to load it from model checkpoints, with a fallback to default configurations. It also migrates InferenceConfig to use Pydantic for parsing, which simplifies the code. The changes are well-structured and include relevant tests for the new loading logic. My feedback focuses on a couple of instances of code duplication that could be refactored to improve maintainability.
- If the config is not present, guess the model version and load an appropriate default instead. This will be updated to include the v2.5 default. - Use Pydantic to parse the InferenceConfig. There is a slight risk that parsing might fail at runtime for something it didn't before, but I couldn't see any problems in our repositories.
oscarkey
force-pushed
the
ok-checkpoint-config
branch
from
2675f1d to
696239c
Compare
![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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bejaeger
left a comment
bejaeger
left a comment
There was a problem hiding this comment.
Very nice! thanks for adding this.
I have mostly minior comments except one, which is about what object we want to version. Shouldn't it be the entire InferenceConfig?
Co-authored-by: Benjamin Jaeger <benjamin@priorlabs.ai>
tests/reference_predictions/inconsistency_test_predictions.json
Outdated
Show resolved
Hide resolved
…present. (#220) * Record copied public PR 576 * Load the inference config from the checkpoint, if present. (#576) - If the config is not present, guess the model version and load an appropriate default instead. This will be updated to include the v2.5 default. - Use Pydantic to parse the InferenceConfig. There is a slight risk that parsing might fail at runtime for something it didn't before, but I couldn't see any problems in our repositories. - I added some unit tests for the loading code and config, but overall this is covered by `test_..._interface.py`, which tries out all the different checkpoints. Co-authored-by: Benjamin Jaeger <benjamin@priorlabs.ai> * Fix tests. * Fix v2.5 and default preprocessing. * Fix tests. --------- Co-authored-by: mirror-bot <mirror-bot@users.noreply.github.com> Co-authored-by: Oscar Key <oscar@priorlabs.ai> Co-authored-by: Benjamin Jaeger <benjamin@priorlabs.ai>
I copied and pasted it wrong before in #576
I copied and pasted it wrong before in #576
test_..._interface.py, which tries out all the different checkpoints.