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

refactor: Remove AnswerToSpeech and DocumentToSpeech nodes #4391

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

silvanocerza
Copy link
Contributor

Proposed Changes:

This PR completely removes AnswerToSpeech and DocumentToSpeech audio nodes and relative tests.

We decided to completely remove them directly as the nodes are not used as shown by our internal metrics.

This nodes will still be usable from the Haystack extras.

How did you test it?

I run tests locally.

Notes for the reviewer

Hold merging this until we publish the extra packages.

@silvanocerza silvanocerza self-assigned this Mar 13, 2023
@silvanocerza silvanocerza requested a review from a team as a code owner March 13, 2023 11:10
@silvanocerza silvanocerza requested review from julian-risch and removed request for a team March 13, 2023 11:10
@julian-risch
Copy link
Member

@agnieszka-m @silvanocerza What's the plan with the documentation? For example with this page: https://docs.haystack.deepset.ai/v1.15-unstable/docs/answer_to_speech
I'd say all the nodes in extras should get a link to the extras repo and a brief explanation that it's not part of Haystack core and how to install. Or will the docs be restructured with an extra section about extras?

@vblagoje
Copy link
Member

Yes, good points @ZanSara I'll, in the meantime, make some minor changes to #4335 . Let's integrate this PR first and then #4335

"pydub",
"protobuf<=3.20.1",
"soundfile< 0.12.0",
"numpy<1.24", # Keep compatibility with latest numba
"openai-whisper"
Copy link
Member

Choose a reason for hiding this comment

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

@agnieszka-m agnieszka-m added the type:documentation Improvements on the docs label Mar 14, 2023
@agnieszka-m
Copy link
Contributor

@julian-risch @silvanocerza so what's the plan for this "extras" repo? what's going to be there?

we definitely need some documentation around it.

But I assume the nodes moved to extras will still work as described in the current docs? just the installation is going to differ?
Maybe we should introduce Haystack core and Haystack extras in the haystack concepts section and then update the installation instructions.

Is this going to be in 1.15?

@silvanocerza
Copy link
Contributor Author

But I assume the nodes moved to extras will still work as described in the current docs? just the installation is going to differ? Maybe we should introduce Haystack core and Haystack extras in the haystack concepts section and then update the installation instructions.

As of now they'll be only moved in the deepset-ai/haystack-extras repo and published as separate packages. Their logic will be left untouched but it's possible it might change in the future. 🤷

I like the idea of introducing the concepts of Haystack core and extras in the installation instructions.

Is this going to be in 1.15?

Yes, they'll be removed already in the next version.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks very good to me already, just a few tiny things that can also be removed:

  • AudioNodeError in haystack/errors.py is unused now and should be removed.
  • AnswerToSpeech and DocumentToSpeech safe_imports should be removed from haystack/nodes/init.py
  • Searching for "audio" in the code base gives ContentTypes = Literal["text", "table", "image", "audio"] in haystack/schema.py and also some results in Embedder. We should add a link to the extras repo maybe, because it relies on it, for example here but looking just at Haystack, it's hard to understand why we still have it.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@silvanocerza
Copy link
Contributor Author

Rebased to fix conflicts.

@silvanocerza silvanocerza merged commit b59cf76 into main Mar 15, 2023
@silvanocerza silvanocerza deleted the audio-nodes-removal branch March 15, 2023 18:31
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.

5 participants