load_csv/feat: support multi csv field

[avelino]

fixed: #146

[walison17]

Suggested change

	def load_csv_and_generate_embeddings(path, cleardb=False, columns=["content"]):
	def load_csv_and_generate_embeddings(path, cleardb=False, columns=("content",)):

[vmesel]

@walison17 why are we fallbacking to a tuple here?

[lgabs]

Actually this part needs to be changed to support this possibility of embedding multiple columns at once (typical case for "question+answer" in the same column (aka a document).

dialog/src/dialog/llm/default.py

Line 41 in f97728a

[f"{c.question}\n{c.content}\n" for c in self.relevant_contents]

[avelino]

Actually this part needs to be changed to support this possibility of embedding multiple columns at once (typical case for "question+answer" in the same column (aka a document).

dialog/src/dialog/llm/default.py

Line 41 in f97728a

[f"{c.question}\n{c.content}\n" for c in self.relevant_contents]

@lgabs the goal of this PR is to generate embeddings with more fields, and not to change the way, we work with prompt generation for OpenAI.

We can discuss this matter in an issue and understand why this behavior should be changed, but this is not the place for that discussion.

[lgabs]

Actually this part needs to be changed to support this possibility of embedding multiple columns at once (typical case for "question+answer" in the same column (aka a document).

dialog/src/dialog/llm/default.py

Line 41 in f97728a

[f"{c.question}\n{c.content}\n" for c in self.relevant_contents]

@lgabs the goal of this PR is to generate embeddings with more fields, and not to change the way, we work with prompt generation for OpenAI.

We can discuss this matter in an issue and understand why this behavior should be changed, but this is not the place for that discussion.

I've said that because we'll probably create confusion: when we allow the embedding of multiple fields, the typical motivation case will be question+answer together, which usually gives much better results in RAG from user queries, and keeping this behavior in default.py will make the prompt generation duplicates the question. We could avoid this confusion explaining that content so far means "answer to a question" and later discuss towards a more concise solution.

[vmesel]

we'll probably create confusion: when we allow the embedding of multiple fields, the typical motivation case will be question+answer together

This is still served as multiple-fields embedding, so we do not need to address to a specific solution, if a user decides the question is not relevant to the context, he should be able to change it easily without editing our source code.

We could avoid this confusion explaining that content so far means "answer to a question" and later discuss towards a more concise solution.

The user can set this into the prompt settings.

[vmesel]

@avelino @walison17 LGTM, but i'd rather have the columns tuple as a list instead of a tuple.

[vmesel]

LGTM!