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

ADR for IBM Granite Embeddings #169

Merged
merged 1 commit into from
Jan 18, 2025
Merged

Conversation

jwm4
Copy link
Contributor

@jwm4 jwm4 commented Dec 20, 2024

This ADR proposes that we will use the IBM Granite Embeddings model for the vector retrieval component of our RAG solution.

@jwm4
Copy link
Contributor Author

jwm4 commented Jan 10, 2025

I received some comments about this proposal in a direct message from @akashgit . The pull request is updated with additional text to address the concerns that he raised.

@jwm4
Copy link
Contributor Author

jwm4 commented Jan 10, 2025

@instructlab/oversight-committee , this has approval from relevant stakeholders and no request for more changes so I think it is ready for final oversight now. Can someone review this and if it meets the criteria merge it too?

Copy link
Member

@danmcp danmcp left a comment

Choose a reason for hiding this comment

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

Please squash commits to remove the intermediate changes

@jwm4
Copy link
Contributor Author

jwm4 commented Jan 17, 2025

Squashing is complete.

@danmcp
Copy link
Member

danmcp commented Jan 18, 2025

@jwm4 Re:

I received some comments about this proposal in a direct message from @akashgit . The pull request is updated with additional text to address the concerns that he raised.

Are you confident these concerns were addressed or do we need @akashgit to re-review?

@jwm4
Copy link
Contributor Author

jwm4 commented Jan 18, 2025

I received some comments about this proposal in a direct message from @akashgit . The pull request is updated with additional text to address the concerns that he raised.

Are you confident these concerns were addressed or do we need @akashgit to re-review?

@akashgit was clear that he doesn't want to be the blocker, so I don't think we need to wait for a re-review. I let him know on January 9 that I had updated the text to address the issues he raised, so if he wanted to submit a formal review in github, I think he would have by now.

He is concerned that we don't have enough data to be really data driven on this issue. I agree with that assessment and I updated the Consequences section to be more explicit about the risk that we will need to pivot to another default as more data emerges. I think that's an acceptable risk because changing the default embedding model is a fairly low cost operation.

I think it is time to make a decision now for what we will do now and then pivot later if it becomes clear that there are better options available. I also think assessing the trade-offs between issues like hardware requirements, accuracy, IP/legal risk, etc. will always be very subjective. I am hoping the "key considerations" listed in this document will provide a framework for making these subjective judgements long after the specific decision about which model is the best one for January 2025 has become irrelevant.

Copy link
Member

@danmcp danmcp left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation!

@danmcp danmcp merged commit b18c310 into instructlab:main Jan 18, 2025
4 checks passed
@jwm4 jwm4 deleted the jwm4-embed-adr branch January 18, 2025 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants