Skip to content

refactor: Refactor HuggingFaceLocalChatGenerator#9455

Merged
sjrl merged 11 commits intomainfrom
ref-hf-local
Jun 13, 2025
Merged

refactor: Refactor HuggingFaceLocalChatGenerator#9455
sjrl merged 11 commits intomainfrom
ref-hf-local

Conversation

@sjrl
Copy link
Copy Markdown
Contributor

@sjrl sjrl commented May 30, 2025 •
edited
Loading

Related Issues

  • fixes #issue-number

Proposed Changes:

  • Refactors internal methods of HuggingFaceLocalChatGenerator to reuse code where possible between the run and run_async methods.
  • This also helps better align the two methods where there were changes made to one but where not also made to the other.
  • Add types

How did you test it?

Existing tests

Notes for the reviewer

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 the type:documentation Improvements on the docs label May 30, 2025
@sjrl sjrl changed the title refactor: Refactor HF Local Chat Generator refactor: Refactor HFLocalChatGenerator May 30, 2025
@sjrl sjrl added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label May 30, 2025
@sjrl sjrl added ignore-for-release-notes PRs with this flag won't be included in the release notes. and removed ignore-for-release-notes PRs with this flag won't be included in the release notes. labels May 30, 2025
@sjrl sjrl changed the title refactor: Refactor HFLocalChatGenerator refactor: Refactor HuggingFaceLocalChatGenerator May 30, 2025
@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented May 30, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 15634575323

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 90.323%
Files with Coverage Reduction New Missed Lines %
components/generators/chat/hugging_face_local.py 12 85.43%
Totals Coverage Status
Change from base Build 15634468219: 0.07%
Covered Lines: 11537
Relevant Lines: 12773
💛 - Coveralls

@sjrl sjrl self-assigned this Jun 4, 2025
@sjrl sjrl marked this pull request as ready for review June 13, 2025 09:49
@sjrl sjrl requested a review from a team as a code owner June 13, 2025 09:49
@sjrl sjrl requested review from Amnah199 and removed request for a team June 13, 2025 09:49
Amnah199
Amnah199 reviewed Jun 13, 2025
View reviewed changes
Comment thread haystack/components/generators/chat/hugging_face_local.py
tool_parsing_function: Optional[Callable[[str], Optional[List[ToolCall]]]] = None,
async_executor: Optional[ThreadPoolExecutor] = None,
):
) -> None:
Copy link
Copy Markdown
Contributor

@Amnah199 Amnah199 Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For my own knowledge, is this now required by linter? I remember having a similar error recently and we didnt specify return types for init in other components.

Copy link
Copy Markdown
Contributor Author

@sjrl sjrl Jun 13, 2025

Choose a reason for hiding this comment

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

Ahh no I don't think it's required right @anakin87 ? At least not yet. It's more just to start getting used to always adding return types to functions.

Copy link
Copy Markdown
Member

@anakin87 anakin87 Jun 13, 2025

Choose a reason for hiding this comment

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

In case there is at least one argument annotated, it's not required. https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods

@sjrl sjrl mentioned this pull request Jun 13, 2025
Merged
Amnah199
Amnah199 reviewed Jun 13, 2025
View reviewed changes
Comment thread haystack/components/generators/chat/hugging_face_local.py
)

# We know it's not None because we check it in _prepare_inputs
assert self.pipeline is not None
Copy link
Copy Markdown
Contributor

@Amnah199 Amnah199 Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

can we use # type: ignore[misc] to replace this like we do in run_async

Copy link
Copy Markdown
Contributor Author

@sjrl sjrl Jun 13, 2025

Choose a reason for hiding this comment

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

True, but I also think it's fine to leave the assert. It's what @anakin87 and others have been doing else where in the code to help mypy

Amnah199
Amnah199 approved these changes Jun 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Amnah199 Amnah199 left a comment

Choose a reason for hiding this comment

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

LG!

@sjrl sjrl merged commit c5027d7 into main Jun 13, 2025
19 checks passed
@sjrl sjrl deleted the ref-hf-local branch June 13, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anakin87 anakin87 anakin87 left review comments

@Amnah199 Amnah199 Amnah199 approved these changes

Assignees

@sjrl sjrl

Labels

ignore-for-release-notes PRs with this flag won't be included in the release notes. type:documentation Improvements on the docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sjrl @coveralls @Amnah199 @anakin87