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

Feature/dynamic rag support #1377

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

tsmith023
Copy link
Contributor

No description provided.

@weaviate-git-bot
Copy link

Great to see you again! Thanks for the contribution.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@tsmith023 tsmith023 marked this pull request as ready for review November 1, 2024 13:22
@tsmith023 tsmith023 requested a review from a team as a code owner November 1, 2024 13:22
Copy link
Collaborator

@dirkkul dirkkul left a comment

Choose a reason for hiding this comment

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

Looks nice!

@@ -18,6 +19,7 @@
__all__ = [
"Filter",
"GeoCoordinate",
"GenerativeProvider",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have this in wvc.generative.XXX?

return _GenerativeOllama(api_endpoint=api_endpoint, model=model, temperature=temperature)

@staticmethod
def openai(
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's have openai and openai_azure

temperature=temperature,
top_p=top_p,
is_azure=is_azure or False,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have a "custom" method? 🤔 Mainly thinking about the generative-dummy module

temperature=temperature,
top_p=top_p,
is_azure=is_azure or False,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would you implement image/video generation for an existing provider? XXX_image(...)?

The prompt to use for RaG on the entire result set.
`grouped_properties`
The properties to use in the RaG on the entire result set.
`dynamic_rag`
Copy link
Collaborator

Choose a reason for hiding this comment

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

name

`single_prompt`
The prompt to use for RaG on each object individually.
`grouped_task`
The prompt to use for RaG on the entire result set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't really use RAG in the python client, but generative.

How about something like
The prompt for the generative query on all results
(same for the others)

@@ -53,6 +55,8 @@ async def bm25(
The prompt to use for RaG on the entire result set.
`grouped_properties`
The properties to use in the RaG on the entire result set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstrings are missing here :)

self._query = _QueryGRPC(
self._connection,
self._name,
self.__tenant,
self.__consistency_level,
validate_arguments=self._validate_arguments,
uses_125_api=self.__uses_125_api,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove this and doing the check in there again?

self.generative_provider = generative_provider

def to_grpc(self, server_version: _ServerVersion) -> generative_pb2.GenerativeSearch:
if server_version.is_lower_than(1, 27, 1): # TODO: change to 1.28.0 when it drops
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we use the is_127_api bool here?

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.

3 participants