-
Notifications
You must be signed in to change notification settings - Fork 433
Implement URI lookup for AI embedding models. #8860
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
base: master
Are you sure you want to change the base?
Conversation
4b9df4c to
5f8e00b
Compare
| ) | ||
|
|
||
| # If this is a generated AI index, track the created type | ||
| generated_ai_model_type = so.SchemaField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to keep the reference? I wonder if you can formulate the generated ref injection via find_extra_refs callback in compile_expr_field instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I am not actually compiling anything into an expression, so I'm not sure that would work.
First, I manually create a qlast.CreateObjectType for the new model type:
Line 1272 in 5f8e00b
| model_ast = qlast.CreateObjectType( |
I compile it:
Lines 1335 to 1339 in 5f8e00b
| model_cmd = sd.compile_ddl( | |
| schema, | |
| model_ast, | |
| ) | |
| model_cmd.set_attribute_value('is_schema_generated', True) |
Then, return to IndexCommand._handle_ai_index_op and make it a prerequisite of the CreateIndex/AlterIndex command:
Line 1025 in 5f8e00b
| self.add_prerequisite(model_cmd) |
However, to get the typeshell of the type produced by the command, I compile the command a second time and actually apply it to the schema:
Lines 1341 to 1355 in 5f8e00b
| # Doing this since using model_cmd or a copy doesn't work | |
| dummy_model_cmd = sd.compile_ddl( | |
| schema, | |
| model_ast, | |
| ) | |
| dummy_model_cmd.set_attribute_value('is_schema_generated', True) | |
| new_schema = dummy_model_cmd.apply(schema, context) | |
| model_type = new_schema.get(model_typename, None, type=s_types.Type) | |
| assert model_type is not None | |
| model_typeshell = so.ObjectShell( | |
| name=model_typename, | |
| schemaclass=type(model_type), | |
| ) | |
| return model_cmd, model_typeshell |
| is_schema_generated = so.SchemaField( | ||
| bool, | ||
| default=False, | ||
| compcoef=0.0, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have similar fields from_alias and from_global. Maybe they can be reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_alias has a lot of associated logic with links and views. I thought it best to avoid adding more meanings to it. @msullivan will hopefully have more insight
aljazerzen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand about half of this, but also don't really understand what it does - even after reading the tests. From what I gather, it allows using AI models from the JSON file in the user schema?
I can spend more time here, but I'd need an explanation.
|
@aljazerzen The issue should explain it #8740. If there is anything in particular you don't understand please ask! I will also add some more comments. |
|
URI selection seems to work great interface-wise. The LSP is not happy though: There were some issues with the Ollama implementation, but that's not the PR to talk about them, so I'll do more research and file them separately. |
|
@anbuzin What sort of issues were you having with ollama? Right now, not all models are in the reference json yet so that might be it. |
|
@dnwpark It was more like, you have to insert a config with an empty string for a secret, and I couldn't get the RAG to work with |
|
@anbuzin Ah yeah, you need to config the provider no matter what. In theory, you could override |
| ) | ||
|
|
||
|
|
||
| class TestExtAIDDL(tb_server.DDLTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests where we add new entries to the manifest, after having done the initial creation.
We should probably only check the file once per server startup (maybe? or in the eventual case maybe once per 24 hours of something).
Since this version doesn't support the http loading anyway, I think the testing approach should be to support passing the file name via an environment variable. Then we can create a temp test dir, start up the server in that directory with start_edgedb_server, do some work, stop it, populate a new updated file, start the server pointing at that instead, and then do the rest of the testing.
Does that make sense?
fb65361 to
12717db
Compare
12717db to
ffbe647
Compare
msullivan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've concluded that I don't like this approach. I think that this will be cleaner if we take a "JSON-forward" approach, where instead of driving the extension worker based on querying the introspection schema, we drive it based on an in-memory datastructure driven by the JSON. It should be pretty easy to produce a data structure for the db-configured stuff with an introspection query; probably we could directly produce the JSON directly using json_object_pack, if we wanted...
One thing that we will need to do either in that new approach or if we wanted to continue this approach, is to cache the last downloaded version of the file. Otherwise, if the fetch fails on server startup or whatever, there will be a degradation of service. In the JSON-forward approach it would be worse, but even in this schema-based approach it would prevent running migration create.
| Mapping[tuple[sn.Name, Optional[str]], uuid.UUID] | ||
| ]=None, | ||
| compat_ver: Optional[verutils.Version] = None, | ||
| reference_paths: Optional[Mapping[str, pathlib.Path]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should pass this in as a mapping to the data. Or if we want to generalize things further, to some abstract interface type for retrieving data?
|
Hm... let me dig into something a bit more. I had forgotten that we actually copy the annotations onto the indexes themselves... but I can't remember why? Edit: Ah, because it's used to make the data more easily accessible in Lines 1468 to 1470 in fe28a9f
I don't really believe that the later lookups would be particularly expensive without it, though. |
|
Here's a query that will generate the exact same JSON I think, from introspecting: Though maybe you'd be better off just doing and postprocessing in python :) |
Related #8740
Allows the definition of ai embedding indexes using a uri:
When parsing a URI, the current schema is checked for a matching model type. If none exists, a reference json is checked and a new type is created if necessary.
For example, if
text-embedding-3-smalldid not exist in the schema, the type__ext_generated_types__::ai_embedding_openai_text-embedding-3-smallwith the appropriate annotations would be generated. This type is automatically cleaned up when the index is deleted.