Skip to content
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

[Inference Client] Add task parameters and a maintenance script of these parameters #2561

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

hanouticelina
Copy link
Contributor

@hanouticelina hanouticelina commented Sep 24, 2024

Fixes #2557.

This PR adds the possibility to pass additional parameters to task methods in the inference client, aligning these task methods with their corresponding parameter specs.

Key changes

  • Add support for additional parameters in the following task methods: audio-classification, document-question-answering, fill-mask, image-classification, image-segmentation, object-detection, question-answering, summarization, text-classification, token-classification, translation and zero-shot-image-classification.
  • Add a semi-automatic script to maintain consistency between task methods and their parameter specs. See Some InferenceClient tasks missing parameters argument, inconsistent with task specifications #2557 for related discussion.
  • Update utils/generate_inference_types.py to add task-specific prefixes to shared type aliases:
    • example: rename ClassificationOutputTransform to TextClassificationOutputTransform for text-classification task
    • this prevents naming conflicts when importing types from different tasks
    • drawbacks of this solution: when you need to make a change that should apply to all tasks, you have to update multiple places
  • Some code refactoring in utils/.
  • Update (automatically) text-to-speech task parameters (following Support VLM in chat completion (+some specs updates) #2556).

Note: automatic-speech-recognition and image-to-text tasks are not included in this update due to an existing parameter naming discrepancy (see issue here).

@HuggingFaceDocBuilderDev

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.

@Wauplin
Copy link
Contributor

Wauplin commented Sep 24, 2024

This PR will complete some tasks from #2063 :)

@hanouticelina
Copy link
Contributor Author

hanouticelina commented Sep 25, 2024

The goal of the script utils/generate_task_parameters.py is :

  1. Based on the schemas defined in ./src/huggingface_hub/inference/_generated/types/, check if there are missing parameters in the InferenceClient task methods.
  2. Update the InferenceClient code by adding the missing parameters to the method signature, update the args section in the docstring and add the necessary imports.

A first implementation was done using ast but it excludes comments and some whitespaces since it focuses only on representing the structure of the code. Instead, we use libCST (github, doc) as it's more suited for our use case :

  • we have lossless round-trip: it allows to parse, modify, and regenerate code without losing any information (it preserves formatting and comments).
  • it provides some nice abstraction to easily identify and work with different parts of the code.

libCST operates on a tree-like structure that represents the entire source code, including whitespace and comments. Here, we leverage two main concepts:

  • Visitors: These are classes that "visit" each node in the tree. It's used in the script to collect information about existing parameters and imports.
  • Transformers: These are similar to visitors but allow us to modify the tree. We use transformers classes to add new parameters, update docstrings, and insert import statements.

The script is still experimental and there are known limitations (non-exhaustive):

  • adding new attributes in the docstring depends heavily of the "format" of the method docstring.
  • some tasks are excluded as they don't follow the pattern inputs/parameters (i.e. there is no Parameters dataclass defined). This is something that we should standardize across all tasks to have an efficient parameters check.
  • it could benefit from more robust error handling.

@hanouticelina hanouticelina marked this pull request as ready for review October 1, 2024 09:57
Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Thanks for the explanations about LibCST. It seems like the right tool to use in our case. Since it's a dev dependency, it's fine adding it. To be honest I did not review the full utils/generate_task_parameters.py script but I'm pretty confident it does the job. Let's hope it'll not become a nightmare to maintain (same for all utils/ scripts^^). If it starts to be the case, we can always reassess. Updating utils is less stressful given we don't have to think about backward compatibility.

Apart from the comments below, I think it's pretty much ready to merge :)

Makefile Outdated Show resolved Hide resolved
) -> List[AudioClassificationOutputElement]:
"""
Perform audio classification on the provided audio content.
For more details about the input parameters, see the [pipeline documentation](https://huggingface.co/docs/transformers/en/main_classes/pipelines#transformers.AudioClassificationPipeline).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit confusing to redirect from Inference docs to the transformers pipeline docs. I feel that we should better document the parameters if that's what's missing here.

Because here for example https://huggingface.co/docs/transformers/en/main_classes/pipelines#transformers.AudioClassificationPipeline.__call__ do not provide any information about function_to_apply, meaning we would have to maintain both docs. Better not to have this additional "docs dependency".

Copy link
Contributor Author

@hanouticelina hanouticelina Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because for almost all task parameters, the default values are only documented in the pipeline docs, e.g. for text-classification here
but you're right, it's still an additional doc dependency but it would be nice to have access to the default values in the inference doc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, we should better document the specs themselves but it will be done in separate PRs 😕

Comment on lines +1503 to +1516
parameters = {
"threshold": threshold,
}
if all(parameter is None for parameter in parameters.values()):
# if no parameters are provided, the image can be raw bytes, an image file, or URL to an online image
data = image
payload: Optional[Dict[str, Any]] = None
else:
# if parameters are provided, the image needs to be a base64-encoded string
data = None
payload = {"inputs": _b64_encode(image)}
for key, value in parameters.items():
if value is not None:
payload.setdefault("parameters", {})[key] = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(out of scope for this PR) I feel that we should factorize the logic to handle inputs + parameters i.e. rules like "base64 encode only if at least 1 parameter", "provide parameters if at least 1 parameter", "provide only not none parameters".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree!

utils/generate_task_parameters.py Outdated Show resolved Hide resolved
utils/generate_task_parameters.py Show resolved Hide resolved
@hanouticelina
Copy link
Contributor Author

@Wauplin do you have an idea why the build of the PR documentation is failing? my guess: private message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some InferenceClient tasks missing parameters argument, inconsistent with task specifications
3 participants