feat: Retrieval augmented generation for chat - #2886
Conversation
|
Thanks! We definitely need to move forward with the depending #2832 which will make the code changes much clearer. |
|
This pull request has merge conflicts that must be resolved before it can be |
cdoern
left a comment
There was a problem hiding this comment.
small preliminary review about some config stuff that is likely giving you issues currently as well as some core norms I would like to follow here if possible!
|
This pull request has merge conflicts that must be resolved before it can be |
5787d80 to
2451694
Compare
cdoern
left a comment
There was a problem hiding this comment.
This looks pretty solid, one or two comments but nothing blocking
| @click.option( | ||
| "--rag", | ||
| "rag_enabled", | ||
| default=False, |
There was a problem hiding this comment.
unsure if you need a default for is_flag types, I think the default is assumed to be false
There was a problem hiding this comment.
I honestly think this is a case where it's good to keep it, much more explicit for folks working with the code
There was a problem hiding this comment.
I expect so too, but there is a minor signal in still explicitly setting default=False to other developers that "we really mean for this to be disabled unless there is good reason otherwise".
| return path.join(self.MODELS_DIR, self.GRANITE_EMBEDDING_MODEL_NAME) | ||
|
|
||
| @property | ||
| def DEFAULT_DOCUMENT_STORE_PATH(self) -> str: |
There was a problem hiding this comment.
we might want to add this to ensure_storage_directories_exist, which is a func that creates all necessary directories before running any ilab cmd
There was a problem hiding this comment.
+1, not blocking as we can do this in one of the other RAG PRs as well
There was a problem hiding this comment.
DEFAULTS._data_dir is already in ensure_storage_directories_exist; the DOCUMENT_STORE_NAME is a filename:
DOCUMENT_STORE_NAME = "embeddings.db"
nathan-weinberg
left a comment
There was a problem hiding this comment.
LGTM as well - great stuff @anastasds!
| @click.option( | ||
| "--rag", | ||
| "rag_enabled", | ||
| default=False, |
There was a problem hiding this comment.
I honestly think this is a case where it's good to keep it, much more explicit for folks working with the code
| return path.join(self.MODELS_DIR, self.GRANITE_EMBEDDING_MODEL_NAME) | ||
|
|
||
| @property | ||
| def DEFAULT_DOCUMENT_STORE_PATH(self) -> str: |
There was a problem hiding this comment.
+1, not blocking as we can do this in one of the other RAG PRs as well
|
@anastasds please squash commits when you get a chance - once done we can remove the |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Anastas Stoyanovsky <astoyano@redhat.com>
|
running large job on this PR, I will then remove the hold |
|
E2E (NVIDIA L40S x4) workflow launched on this PR: View run |
|
e2e workflow succeeded on this PR: View run, congrats! |
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
Resolves #2876
Depends on #2832
Dev doc: instructlab/dev-docs#161
Checklist:
conventional commits.