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

fix: serialize/deserialize torch dtype in the components that need it #6713

Merged
merged 15 commits into from
Jan 12, 2024

Conversation

anakin87
Copy link
Member

@anakin87 anakin87 commented Jan 9, 2024

Related Issues

Proposed Changes:

  • In each affected component, convert the torch.dtype parameters to string during serializaion.
  • Convert back to torch.dtype during deserialization

How did you test it?

CI, extended unit tests, new tests

Notes for the reviewer

If you feel that there is too much code duplication, I can put the serialization/deserialization logic into a utility function.

Checklist

@anakin87 anakin87 changed the title fix: serialize/deserialize torch dtype in the components that need id fix: serialize/deserialize torch dtype in the components that need it Jan 9, 2024
@github-actions github-actions bot added topic:tests 2.x Related to Haystack v2.0 type:documentation Improvements on the docs labels Jan 9, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jan 9, 2024

Pull Request Test Coverage Report for Build 7501213766

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 87.121%

Totals Coverage Status
Change from base Build 7490996754: 0.05%
Covered Lines: 4282
Relevant Lines: 4915

💛 - Coveralls

@anakin87 anakin87 requested a review from masci January 10, 2024 10:21
@anakin87 anakin87 marked this pull request as ready for review January 10, 2024 10:21
@anakin87 anakin87 requested review from a team as code owners January 10, 2024 10:21
@anakin87 anakin87 requested review from dfokina and silvanocerza and removed request for a team and silvanocerza January 10, 2024 10:21
@anakin87 anakin87 requested a review from sjrl January 11, 2024 14:18
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Left one comment that's not actually related to the main point of the PR.

About code duplication, having similar code across 3 components I think it's fine, we can extract common utilities as we need them for more components.

haystack/components/generators/hugging_face_local.py Outdated Show resolved Hide resolved
@anakin87 anakin87 requested a review from masci January 12, 2024 10:47
@anakin87 anakin87 merged commit 80c3e68 into main Jan 12, 2024
22 checks passed
@anakin87 anakin87 deleted the serialize-torch-dtype branch January 12, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to Haystack v2.0 topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dumping a Pipeline to yaml is broken for certain kwargs
4 participants