refactor: remove model-associated inference params #272
Conversation
crates/llm-base/src/lib.rs
Outdated
| impl InferenceParameters { | ||
| /// Returns a reasonable default for the parameters. | ||
| /// | ||
| /// Note that these parameters are not necessarily optimal for all models, and that | ||
| /// you may want to tweak them for your use case. | ||
| /// | ||
| /// This is intentionally not a `Default` implementation. The values specified here may change | ||
| /// in the future, and we want to make sure that users are aware of this and do not accidentally | ||
| /// rely on the values. | ||
| pub const fn reasonable_default() -> Self { |
There was a problem hiding this comment.
This feels unnecessary to me. Is there anything that states that a default implementation should be expected to be optimal in all circumstances? It also seems like semantic versioning should be used to take care of breaking changes...choosing a different name for something that will effectively still be used the same way isn't going to help anything. This isn't a hill I'm going to die on, but I just have a strong preference for the use of idiomatic traits.
There was a problem hiding this comment.
I see where you're coming from and I'm not super-sure about this myself, but I wanted to deliberately disable the use of Default so that the user can't just smash Default::default in without thinking about it at all.
But perhaps you're right, and there's not really that much distinction in practice. I'll think about it...
There was a problem hiding this comment.
IMO the "correct" way to approach it would be to add documentation to InferenceParameter, InferenceRequest and infer that specifies that it's highly discouraged to use default InferenceParameters in production/performance-sensitive situations 🤷🏻
philpax
force-pushed
the
remove-model-inference-params
branch
from
e651744 to
b4a4b16
Compare
While well-intentioned, the presence of the parameters on the model suggests to users that these are parameters optimised for the model.
Unfortunately, that's not quite what they are in practice - they're the parameters passed into the load function, which the vast majority of users will call with
Default::default.I've attacked this with two changes:
Defaultimplementation forInferenceParametersand moved it to areasonable_defaultmethod, to make it clearer that this is something that you as a user will need to think about and can't idly hide in aDefault::defaultcall somewherePlease let me know if this is the right call to make.