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

Make embeddings optional in weaviate component #791

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Make embeddings optional in weaviate component #791

merged 5 commits into from
Jan 18, 2024

Conversation

PhilippeMoussalli
Copy link
Contributor

PhilippeMoussalli
PhilippeMoussalli commented Jan 17, 2024
View reviewed changes
components/index_weaviate/fondant_component.yaml
type: array
items:
type: float32
additionalProperties: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To accomodate for the fact that we now want to accept optional embedding we had to make this change. For the end user, it's now not very clear what the component can accept or how it behaves unless they look at the source code or encounter an example where it's implemented. It's fine as a temporary solution but still a fan of implementing something as specified in this discussion

#567 (comment)

Curious to hear other takes on this

Copy link
Member

Choose a reason for hiding this comment

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

Agree that we should still add this in the future. For now, you can document this in the description. It supports markdown.

Base automatically changed from enable-testing-weaviate to main January 17, 2024 16:39
RobbeSneyders
RobbeSneyders approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks!

components/index_weaviate/fondant_component.yaml
type: array
items:
type: float32
additionalProperties: true
Copy link
Member

Choose a reason for hiding this comment

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

Agree that we should still add this in the future. For now, you can document this in the description. It supports markdown.

@PhilippeMoussalli PhilippeMoussalli merged commit 538aa63 into main Jan 18, 2024
1 of 2 checks passed
@PhilippeMoussalli PhilippeMoussalli deleted the make-embedding-optional-weaviate-index branch January 18, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobbeSneyders RobbeSneyders RobbeSneyders approved these changes

@mrchtr mrchtr Awaiting requested review from mrchtr

@GeorgesLorre GeorgesLorre Awaiting requested review from GeorgesLorre

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make embedding optional
2 participants
@PhilippeMoussalli @RobbeSneyders