-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[InferenceClient] Better handling of task parameters #2812
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested anything locally but it looks good to me. Thanks for handling these discrepancies!
@@ -2407,7 +2405,7 @@ def text_to_image( | |||
scheduler: Optional[str] = None, | |||
target_size: Optional[TextToImageTargetSize] = None, | |||
seed: Optional[int] = None, | |||
**kwargs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change but hopefully totally fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll mention this in the next release notes! but it should be fine, if users were previously using text_to_image
with the HF Inference API, this shouldn't be an issue since all API parameters were exposed as explicit method arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be an issue since all API parameters were exposed as explicit method arguments
yes exactly
@@ -2443,6 +2441,9 @@ def text_to_image( | |||
The size in pixel of the output image | |||
seed (`int`, *optional*): | |||
Seed for the random number generator. | |||
extra_parameters (`Dict[str, Any]`, *optional*): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a good example of how to use extra_parameters
for a specific model on a specific provider? Would be good to add a least one example (either in text-to-image or text-to-video depending on what's best)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, added in 305c720
@pytest.mark.parametrize( | ||
"helper,inputs,parameters,expected_data,expected_json", | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding these!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@@ -134,7 +134,7 @@ def __init__(self): | |||
|
|||
def _prepare_payload(self, inputs: Any, parameters: Dict[str, Any]) -> Dict[str, Any]: | |||
parameters = {k: v for k, v in parameters.items() if v is not None} | |||
if "image_size" not in parameters and "width" in parameters and "height" in parameters: | |||
if "width" in parameters and "height" in parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if only one if passed btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to send only one if specified, the other one would be set to the default value. I'll fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actually for fal-ai, you either send both, or neither.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no default values for each one of them, according to their documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks for checking 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving, though I'm not sure to understand what's happening in utils/generate_inference_types.py
. I trust it we result is correct :)
should be good now, let's merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) i think OpenAI calls this param extra_body
(https://docs.vllm.ai/en/latest/serving/openai_compatible_server.html#extra-parameters or from the Python openai sdk), but i think our name is probably fine too 🤷
Then I'm down to go with |
up to you (i'm not sure it's exactly the same use case, but my brain is fried) |
It's actually the same use case, you can send an extra parameter using openai client, but i'm not sure if it's |
extra_query goes into the URL query params no? (just guessing) |
yep, just wanted to double check in their docstrings, since it's not really documented. |
This PR:
text-to-image
parameters (particularly for Together AI).extra_parameters
argument totext_to_image
,text_to_speech
andtext_to_video
to be able to pass a provider's unique parameters for these tasks.