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

feat: support for tools in HuggingFaceAPIChatGenerator #8661

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Conversation

anakin87
Copy link
Member

@anakin87 anakin87 commented Dec 19, 2024

Related Issues

Proposed Changes:

  • basically a porting of HuggingFaceAPIChatGenerator from experimental
  • there are some slight changes, that I will try to comment in this PR to make them visible

How did you test it?

CI, several new tests

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@github-actions github-actions bot added type:documentation Improvements on the docs topic:tests labels Dec 19, 2024
@coveralls
Copy link
Collaborator

coveralls commented Dec 19, 2024

Pull Request Test Coverage Report for Build 12413858648

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.04%) to 90.693%

Files with Coverage Reduction New Missed Lines %
components/generators/chat/hugging_face_api.py 4 96.4%
utils/hf.py 7 86.29%
Totals Coverage Status
Change from base Build 12410533994: 0.04%
Covered Lines: 8351
Relevant Lines: 9208

💛 - Coveralls

@@ -216,6 +216,19 @@ def _remove_title_from_schema(schema: Dict[str, Any]):
del property_schema[key]


def _check_duplicate_tool_names(tools: List[Tool]) -> None:
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a common check we do, so I moved it to the Tool module

@@ -270,6 +270,44 @@ def check_generation_params(kwargs: Optional[Dict[str, Any]], additional_accepte
)


def convert_message_to_hf_format(message: ChatMessage) -> Dict[str, Any]:
Copy link
Member Author

Choose a reason for hiding this comment

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

We should use this function in the HF Local Chat Generator too (I recently discovered that it does not work properly without the conversion), so I moved it to utils.

(I will open issue + PR to update the HF Local Chat Generator.)

@anakin87 anakin87 marked this pull request as ready for review December 19, 2024 10:55
@anakin87 anakin87 requested review from a team as code owners December 19, 2024 10:55
@anakin87 anakin87 requested review from dfokina, Amnah199 and vblagoje and removed request for a team and Amnah199 December 19, 2024 10:55
@@ -14,7 +14,7 @@
from haystack.utils.hf import HFEmbeddingAPIType, HFModelType, check_valid_model
from haystack.utils.url_validation import is_valid_http_url

with LazyImport(message="Run 'pip install \"huggingface_hub>=0.23.0\"'") as huggingface_hub_import:
with LazyImport(message="Run 'pip install \"huggingface_hub>=0.27.0\"'") as huggingface_hub_import:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is only a suggestion for users.
The ChatCompletionInputTool type was introduced recently, so it is better to install the latest version.

Copy link
Member

@vblagoje vblagoje 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, I left a a few optional comments

@@ -159,6 +163,11 @@ def __init__( # pylint: disable=too-many-positional-arguments
msg = f"Unknown api_type {api_type}"
raise ValueError(msg)

if tools:
if streaming_callback is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Should we do this across the board for all CG - just something to write down and not forget

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Most of Generators support Tools + Streaming, I think.

Theoretically also HF could support this but it is highly undocumented and does not behave consistently.
See deepset-ai/haystack-experimental#120 (comment)

"finish_reason": choice.finish_reason,
"index": choice.index,
"usage": {
"prompt_tokens": api_chat_output.usage.prompt_tokens,
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I've been burned b4 on these usage structures assuming they are always there. Please check HF pydantic classes to make sure they are not optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have now taken a more cautious approach. Thank you!

@@ -216,6 +216,19 @@ def _remove_title_from_schema(schema: Dict[str, Any]):
del property_schema[key]


def _check_duplicate_tool_names(tools: List[Tool]) -> None:
"""
Check for duplicate tool names.
Copy link
Member

Choose a reason for hiding this comment

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

and raises ValueError if they are found.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

@anakin87 anakin87 merged commit 2bc58d2 into main Dec 19, 2024
19 checks passed
@anakin87 anakin87 deleted the hfapi-tools branch December 19, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants