✨ Text generation input inference data models#151
✨ Text generation input inference data models#151tharapalanivel wants to merge 2 commits intocaikit:mainfrom
Conversation
Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>
tharapalanivel
requested review from
alex-jw-brooks,
evaline-ju and
gabe-l-hart
as code owners
Signed-off-by: Thara Palanivel <130496890+tharapalanivel@users.noreply.github.com>
| top_k: int | ||
| top_p: int | ||
| typical_p: float | ||
| seed: Optional[int] |
There was a problem hiding this comment.
It might be a good idea to set some default! TGIS defaults are here.
Most of the time this doesn't matter, because 0 temperature (in the IBM fork) indicates greedy decoding, so top_k, top_p, typical_p, etc won't be used, as they're sampling only.
TGI doesn't use temperature 0 as a toggle though, so it would be also be nice in case those APIs are ever more unified - currently there are some small divergences with stuff like prompt IDs. I'm not sure if our raw generation modules are compatible with it or not
There was a problem hiding this comment.
Haven't seen us setting defaults on the data models themselves, only in the inference methods. I don't really have a strong opinion on this, trying to understand if that is the general direction caikit is moving in
There was a problem hiding this comment.
I think even if we set defaults on the DM, they won't propagate to proto, so the default here would be guided by the .run function themselves.
There was a problem hiding this comment.
Good point - my main concern with leaving it up to run is that it's easy for defaults to get out of sync if we have multiple modules relying on them.
I guess an alternate is to either have a building for getting these objects with their default values that make sense, or to have consts be passed to the run function 🤔 is the intent with this type to have a parameter that is this DM object type, or to take primitives and build this object in the requests?
|
We decided to go with flattened API for now. This may be revisited, so we are keeping this PR open |
3 participants
Supports #140