Conversation
ilya-kolchinsky
left a comment
ilya-kolchinsky
left a comment
There was a problem hiding this comment.
Please fix the remarks.
|
|
||
|
|
||
| def index_path_for_rag(path, **kwargs): | ||
| def create_index_pipeline(path, **kwargs): |
There was a problem hiding this comment.
At the API level, I believe we still want to have a unified "one-shot" method that handles both pipeline creation and execution. It will fit cases like RamaLama where simplicity is the top priority.
This is especially true for indexing which is typically done in a single stage as opposed to querying the model. But same applies for execute_rag_query as well.
| 1) Indexing mode (-i flag) - index a collection of documents from the given path. | ||
| 2) RAG query mode (-r flag) - answer a given query with RAG using the previously indexed documents. | ||
| 3) Evaluation mode (-e flag) - evaluate the RAG pipeline as specified in the settings - NOT YET OFFICIALLY SUPPORTED. | ||
| 1) Index pipeline creation mode (-ip flag) - create an indexing pipeline from the given path. |
There was a problem hiding this comment.
I'm really not sure these new modes should be exposed via main.py. What would a user do with a pipeline that is immediately destroyed after the program exits? IMO, this new functionality should be reserved for API only. At some future point we might introduce a more advanced main.py that interacts with the user via command line similarly to ilab chat - then these new flags would make sense.
This is of course only my opinion let's discuss if you think otherwise :)
| def run(self): | ||
| logger.debug(f"Executing the pipeline with the following arguments:\n{self._args}") | ||
| return self._pipeline.run(self._args) | ||
| def run(self, pipeline_args_dict=None): |
There was a problem hiding this comment.
So at pipeline creation we provide the default values for the run that are stored under self._args. Your new implementation keeps this self._args and either uses it or alternatively a dictionary provided as input. What if the caller only can/wants to provide a subset of the parameters? In this case, the two sources of parameters should be merged and the merge result should be used as input for the Haystack pipeline.
Another important point is whether we should keep the defaults provided at initialization. Wouldn't it be better to override the defaults (that will mostly be arbitrary anyway) with the new inputs? Perhaps add a Boolean parameter for run() to control that?
| return result["answer_builder"]["answers"][0] | ||
|
|
||
| return result["llm"]["replies"][0] | ||
| else: #not in eval mode |
There was a problem hiding this comment.
While this approach obviously works, it would be very challenging to maintain this code long-term going forward. It basically hard-codes the structure of the pipeline. Any attempt to introduce even a slightly different pipeline in the future will make this thing explode.
One way to resolve this would be to initialize the relevant parameters to None at pipeline creation (in all places where you removed them), then to rely on the code in the very beginning of the run() method to replace all the relevant fields with the up-to-date query. Please let me know if you'd like to handle it differently or if you have any concerns in this regard, and we will discuss.
2 participants
(Addresses #28)
This PR allows to build a reusable indexing and RAG pipeline.
Main Changes:
rag.pyto allow therun()function to execute the pipeline with required component argumentspipeline.pyto allow the component arguments to be passed as a dictionary for the RAG pipelineapi.pyto introduce separate functions for building and executing the index/RAG pipelinestest/sanity_test.pyto demonstrate these changesTesting the Code
test/sanity_test.pyto test all the new implemented changes