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.

Sign up for GitHub

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

Jump to bottom

LocalWhisperTranscriber (v2) #4909

Merged
merged 20 commits into from
May 22, 2023
Merged

LocalWhisperTranscriber (v2) #4909

merged 20 commits into from
May 22, 2023

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented May 12, 2023 •
edited
Loading

Related Issues

Proposed Changes:

  • Adds LocalWhisperTranscriber
  • Adds its unit tests and a e2e test

How did you test it?

  • Local test runs
  • CI

Notes for the reviewer

n/a

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels May 12, 2023
@ZanSara ZanSara linked an issue May 15, 2023 that may be closed by this pull request
Port WhisperTranscriber to v2 pipelines #4837
Closed
@ZanSara ZanSara marked this pull request as ready for review May 15, 2023 09:39
@ZanSara ZanSara requested a review from a team as a code owner May 15, 2023 09:39
@ZanSara ZanSara requested review from masci and silvanocerza and removed request for a team May 15, 2023 09:39
@coveralls
Copy link
Collaborator

coveralls commented May 15, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5047225682

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 39.529%
Totals Coverage Status
Change from base Build 5046746144: 0.1%
Covered Lines: 8868
Relevant Lines: 22434
💛 - Coveralls

masci
masci suggested changes May 15, 2023
View reviewed changes
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.

Code looks good but I would simplify the testing strategy

haystack/preview/components/audio/whisper_local.py Outdated Show resolved Hide resolved
haystack/preview/components/audio/whisper_local.py Outdated Show resolved Hide resolved
test/preview/components/test_whisper_local.py Outdated Show resolved Hide resolved
test/preview/components/test_whisper_local.py Outdated Show resolved Hide resolved
test/preview/components/test_whisper_local.py Outdated Show resolved Hide resolved
test/preview/components/test_whisper_local.py Outdated Show resolved Hide resolved
test/preview/components/test_whisper_local.py Outdated Show resolved Hide resolved
masci
masci suggested changes May 15, 2023
View reviewed changes
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.

Code looks good but I would simplify the testing strategy

@ZanSara ZanSara requested a review from masci May 15, 2023 12:42
@ZanSara ZanSara requested a review from masci May 16, 2023 17:12
masci
masci approved these changes May 22, 2023
View reviewed changes
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.

LGTM!

@masci masci merged commit f80ae01 into main May 22, 2023
@masci masci deleted the v2-whisper-local branch May 22, 2023 16:30
@masci masci added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label May 23, 2023
@ZanSara ZanSara mentioned this pull request Jul 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masci masci masci approved these changes

@silvanocerza silvanocerza Awaiting requested review from silvanocerza

Assignees
No one assigned
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port WhisperTranscriber to v2 pipelines
3 participants
@ZanSara @coveralls @masci