feat: Ingest document embeddings for Retrieval augmented generation by dmartinol · Pull Request #2903 · instructlab/instructlab

dmartinol · 2025-01-10T15:45:39Z

Issue resolved by this Pull Request:
Resolves #2875
Addresses #2957
Depends on #2832

Dev Docs related to this Pull Request:
Link to Dev Doc or PR: instructlab/dev-docs#161

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the
conventional commits.
Changelog updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Functional tests have been added, if necessary.
E2E Workflow tests have been added, if necessary.

Unit test code in the next commit

dmartinol · 2025-01-15T08:52:30Z

test coverage:

src/instructlab/cli/rag/__init__.py                                0      0   100%
src/instructlab/cli/rag/ingest.py                                 30      3    90%
src/instructlab/cli/rag/rag.py                                    13      2    85%
src/instructlab/rag/__init__.py                                    0      0   100%
src/instructlab/rag/document_store.py                              7      0   100%
src/instructlab/rag/document_store_factory.py                      9      0   100%
src/instructlab/rag/haystack/component_factory.py                 22      0   100%
src/instructlab/rag/haystack/components/document_splitter.py      52      7    87%
src/instructlab/rag/haystack/document_store_factory.py             9      0   100%
src/instructlab/rag/haystack/document_store_ingestor.py           42      3    93%
src/instructlab/rag/haystack/document_store_retriever.py          27      0   100%
src/instructlab/rag/taxonomy_utils.py                             16      0   100%

cdoern

nothing jumps out at me, just one comment

dmartinol · 2025-01-22T08:06:59Z

@anastasds I integrated changes for RAG CLI.
Note that I renamed the embedding-model-name argument to embedding-model-path as we agreed in the slack channel with @jwm4 (applied to all chat modules as well)

cdoern

This looks good! I think we can always iterate on this over time, but I'd like to get this in so we can add e2e testing on top of all the new RAG features.

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>

**Issue resolved by this Pull Request:** Resolves #2957 Changes related to `ingest` CLI will be added to #2903 **Checklist:** - [x] **Commit Message Formatting**: Commit titles and messages follow guidelines in the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary). - [ ] [Changelog](https://github.com/instructlab/instructlab/blob/main/CHANGELOG.md) updated with breaking and/or notable changes for the next minor release. - [ ] Documentation has been updated, if necessary. - [ ] Unit tests have been added, if necessary. - [ ] Functional tests have been added, if necessary. - [ ] E2E Workflow tests have been added, if necessary. Approved-by: cdoern Approved-by: nathan-weinberg

alinaryan

LGTM! one non blocking nit

alinaryan · 2025-01-22T15:17:43Z

 @clickext.display_params
 def convert(
-    ctx,
+    ctx,  # pylint: disable=unused-argument


nit: unless it's required for some other reason, you should be able to remove ctx, the pylint comment and @click.pass_context

Since I did not develop the command but just hit this linting issue (not sure why passed the CI), I preferred (for now) to just solve it this way.

FWIW, I think removing the arg would be better. I accidentally left it in after getting rid of the code that used it.

(But I am fine saying that should go into a future PR and not this one if you prefer)

Related ADR: instructlab/dev-docs#178 Depends on #2886 , #2903 Resolves #2888 * Adds feature gating following the pattern in the ADR linked above * Adds an `@dev_preview` decorator that mocks the feature scope environment variable for specific tests covering experimental functionality. * Adds unit tests verifying that RAG related functionality gives a useful and accurate error message when attempted to be used without being enabled. Approved-by: nathan-weinberg Approved-by: cdoern

mergify Bot added testing Relates to testing dependencies Relates to dependencies ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 10, 2025

dmartinol marked this pull request as ready for review January 10, 2025 20:31

mergify Bot added the ci-failure PR has at least one CI failure label Jan 10, 2025

reidliu41 reviewed Jan 11, 2025

View reviewed changes

Comment thread src/instructlab/defaults.py Outdated

reidliu41 reviewed Jan 11, 2025

View reviewed changes

Comment thread src/instructlab/defaults.py Outdated

dmartinol force-pushed the rag_ingest branch from 8fccead to 4db0f55 Compare January 13, 2025 15:00

mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 13, 2025

dmartinol commented Jan 13, 2025

View reviewed changes

Comment thread tests/testdata/default_config.yaml Outdated

mergify Bot removed the ci-failure PR has at least one CI failure label Jan 14, 2025

dmartinol force-pushed the rag_ingest branch from bd24683 to c8f65fe Compare January 14, 2025 10:13

mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 14, 2025

dmartinol mentioned this pull request Jan 14, 2025

feat(rag): Add lab rag convert CLI command #2902

Merged

6 tasks

dmartinol force-pushed the rag_ingest branch from 17727a9 to 6e2ba8b Compare January 15, 2025 07:20

mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 15, 2025

dmartinol force-pushed the rag_ingest branch from d2bdcb4 to 751de99 Compare January 16, 2025 08:20

mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 16, 2025

mergify Bot added the ci-failure PR has at least one CI failure label Jan 21, 2025

This was referenced Jan 21, 2025

Feature gates #2946

Merged

feat: Add end to end tests for RAG-enabled chat using user-supplied documents #2948

Closed

mergify Bot removed the ci-failure PR has at least one CI failure label Jan 21, 2025

dmartinol force-pushed the rag_ingest branch from 05e8da7 to 0711ec6 Compare January 21, 2025 16:57

mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 21, 2025

nathan-weinberg reviewed Jan 21, 2025

View reviewed changes

Comment thread src/instructlab/cli/rag/ingest.py Outdated

Comment thread src/instructlab/cli/rag/ingest.py

cdoern reviewed Jan 21, 2025

View reviewed changes

Comment thread src/instructlab/rag/taxonomy_utils.py

Comment thread tests/testdata/default_config.yaml Outdated

dmartinol force-pushed the rag_ingest branch 2 times, most recently from f52da9b to 4601aeb Compare January 21, 2025 21:54

nathan-weinberg approved these changes Jan 21, 2025

View reviewed changes

mergify Bot added one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jan 21, 2025

dmartinol force-pushed the rag_ingest branch from 4601aeb to 6f56636 Compare January 22, 2025 07:26

mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025

dmartinol force-pushed the rag_ingest branch from d45f246 to 8a2187a Compare January 22, 2025 08:05

dmartinol mentioned this pull request Jan 22, 2025

fix: Use lazy import to not impact the CLI layer #2959

Merged

6 tasks

dmartinol force-pushed the rag_ingest branch from 8a2187a to d12f3f5 Compare January 22, 2025 13:48

cdoern approved these changes Jan 22, 2025

View reviewed changes

mergify Bot added ci-failure PR has at least one CI failure and removed one-approval PR has one approval from a maintainer labels Jan 22, 2025

RAG ingest CLI

74b8ef3

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>

dmartinol force-pushed the rag_ingest branch from d12f3f5 to 74b8ef3 Compare January 22, 2025 15:01

mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025

alinaryan approved these changes Jan 22, 2025

View reviewed changes

mergify Bot merged commit 3db4dd4 into instructlab:main Jan 22, 2025

Navigation Menu

Uh oh!

feat: Ingest document embeddings for Retrieval augmented generation - #2903

feat: Ingest document embeddings for Retrieval augmented generation#2903
mergify[bot] merged 1 commit into
instructlab:mainfrom
dmartinol:rag_ingest

dmartinol commented Jan 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmartinol commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

cdoern left a comment

Uh oh!

Uh oh!

Uh oh!

dmartinol commented Jan 22, 2025

Uh oh!

cdoern left a comment

Uh oh!

alinaryan left a comment

Uh oh!

alinaryan Jan 22, 2025

Uh oh!

dmartinol Jan 22, 2025

Uh oh!

jwm4 Jan 22, 2025

Uh oh!

jwm4 Jan 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

dmartinol commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmartinol commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmartinol commented Jan 22, 2025

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

alinaryan left a comment

Choose a reason for hiding this comment

Uh oh!

alinaryan Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

dmartinol Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

jwm4 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

jwm4 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dmartinol commented Jan 10, 2025 •

edited

Loading