huggingface / huggingface_hub

The official Python client for the Huggingface Hub.
https://huggingface.co/docs/huggingface_hub
Apache License 2.0
2.12k stars 556 forks source link

To unify `text_generation()`'s `stop_sequences` and `TextGenerationInput`'s `stop` params #2471

Closed sadra-barikbin closed 2 months ago

sadra-barikbin commented 3 months ago

Hi there! 🤗

It would be great if text_generation()'s stop_sequences and TextGenerationInput's stop params are unified just like chat_completion and ChatCompletionInput to be able to feed the input dataclasses to the client methods directly:

client.text_generation(prepared_request.inputs, **prepared_request.parameters) # currently raises `TypeError: text_generation() got an unexpected keyword argument 'stop'`
# or
client.chat_completion(**prepared_request)

https://github.com/huggingface/huggingface_hub/blob/359093fbe4fa049aadadf08ca60a2b0fa9a39c2d/src/huggingface_hub/inference/_client.py#L1658

sadra-barikbin commented 3 months ago

Currently I should do this as the alternative:

from dataclasses import asdict

input_as_dict = asdict(input)
input_as_dict["parameters"]["stop_sequences"] = input_as_dict["parameters"]["stop"]
del input_as_dict["parameters"]["stop"]
client.text_generation(input.inputs, **input_as_dict["parameters"])
Wauplin commented 3 months ago

Hi @sadra-barikbin, yes I definitely agree this parameter should be unified. I opened a PR to fix it :) https://github.com/huggingface/huggingface_hub/pull/2473

sadra-barikbin commented 3 months ago

Thanks @Wauplin ! 👍

sadra-barikbin commented 3 months ago

There's also a discrepancy between ChatCompletionInput's stream param which is optional defaulting to None and chat_completion()'s stream param which should be either True or False. Is this OK?

https://github.com/huggingface/huggingface_hub/blob/359093fbe4fa049aadadf08ca60a2b0fa9a39c2d/src/huggingface_hub/inference/_generated/types/chat_completion.py#L123

Wauplin commented 3 months ago

Thanks for flagging this @sadra-barikbin. It's a slight discrepancy but it allows us to simplify the @overload logic in InferenceClient.chat_completion. If we add the type annotation for None value, we would have to overload the method even more. The problem is that bool and Literal[True, False] is not considered the same by mypy / code editors. It led to quite some headaches when trying to solve that (the main thing I was focused on was to have correct autocomplete for the lib'). So all of this to say, the slight discrepancy is fine for now especially because it doesn't change anything in this case.