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: support meta routing of Answers #6539

Closed
wants to merge 8 commits into from
Closed

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Dec 13, 2023

Related Issues

This PR implements supporting anwers with funcitonality already implemented for documents:

  • language classification
  • metadata routing
  • joining

Proposed Changes:

  • add common type-check protocol MetaDataclass
  • make MetadataRouter work with answers and documents
  • add AnswerLanguageClassifier and AnswerJoiner

How did you test it?

  • added unittets

Notes for the reviewer

Checklist

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Dec 13, 2023
@silvanocerza
Copy link
Contributor

Good idea indeed but I think having a MetaDataclass protocol is a bit overkill, we can just check if an object has the meta field with hasfield(obj, "meta").

It's also quite problematic using the MetaDataclass as output in MetadataRouter, it makes it hard to connect to other components. The output could be either Answer, Document, or ByteStream but a Component that takes Document as input won't accept the connection with MetadataRouter this way.

I'm also wondering whether it makes sense to have different classes to manipulate Answer and Document. Look at the AnswerLanguageClassifier as an example, it's basically identical to DocumentLanguageClassifier apart from the I/O types.

Not saying I'm against it, just wondering whether it's worth duplicating the code. As of now it's not a huge problem since the code is identical apart from the types, but what if we need to differentiate something in the future given the type?

Let's think a bit about this. 🤔

@masci
Copy link
Contributor

masci commented May 9, 2024

Closing as very outdated, feel free to reopen if it's still relevant

@masci masci closed this May 9, 2024
@masci masci deleted the feat/answer_meta_routing 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
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants