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: Add ByteStream and ChatMessage to Haystack init #6522

Closed
wants to merge 1 commit into from

Conversation

julian-risch
Copy link
Member

Related Issues

I noticed when reviewing this PR that current imports of Document and ChatMessage or ByteStream differ.

from haystack import Answer, Document, ExtractedAnswer, GeneratedAnswer
from haystack.dataclasses.byte_stream import ByteStream
from haystack.dataclasses.chat_message import ChatMessage

As all of these are user facing and not just for internal use, as a user I would expect all of to work with

from haystack import Answer, Document, ExtractedAnswer, GeneratedAnswer, ByteStream, ChatMessage

We shouldn't treat them differently.

Proposed Changes:

  • Added ByteStream and ChatMessage imports to Haystack's init.py

How did you test it?

Notes for the reviewer

init.py gets longer with this change but I don't see a reason why we would treat some dataclasses different than others. What do you think?

Checklist

@julian-risch julian-risch requested a review from a team as a code owner December 11, 2023 13:54
@julian-risch julian-risch requested review from silvanocerza and removed request for a team December 11, 2023 13:54
@julian-risch julian-risch added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Dec 11, 2023
@silvanocerza
Copy link
Contributor

I wouldn't do this to be fair.

I already discussed a bit with @masci regarding this and we both agree that we should stop exporting everything at the root.

We should remove and not add from this __init__.py, or we risk the same situation we have for 1.x that importing haystack takes way too much time.

@julian-risch
Copy link
Member Author

@silvanocerza Okay, I see. Yes the speed is the only downside I could think of. Have you considered removing ExtractiveAnswer from the init? Or any of the other dataclasses currently in there?

@silvanocerza
Copy link
Contributor

I would actually remove most of the imports from there. I would probably leave just Pipeline. 🤔
But even nothing is good in my opinion.

@masci masci deleted the bytestream-chatmessage-init-import branch June 18, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants