CrateDB vector: Improve SQLAlchemy model factory #13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amotl
force-pushed
the
full-dynamic-schema
branch
from
bc43234
to
e2a98c9
Compare
amotl
changed the title
From now on, _all_ instances of SQLAlchemy model types will be created
at runtime through the `ModelFactory` utility.
By using `__table_args__ = {"keep_existing": True}` on the ORM entity
definitions, this seems to work well, even with multiple invocations
of `CrateDBVectorSearch.from_texts()` using different `collection_name`
argument values.
While being at it, this patch also fixes a few linter errors.
amotl
force-pushed
the
full-dynamic-schema
branch
from
e2a98c9
to
c76dc83
Compare
amotl
requested review from
hammerhead,
matriv and
mkleen
and removed request for
seut,
matriv and
hammerhead
This was referenced Nov 21, 2023
amotl
pushed a commit
that referenced
this pull request
Mar 28, 2024
…e `text-splitter` directory is copied before poetry installation (langchain-ai#19214) ## Description This PR modifies the settings in `libs/langchain/dev.Dockerfile` to ensure that the `text-splitters` directory is copied before the poetry installation process begins. Without this modification, the `docker build` command fails for `dev.Dockerfile`, preventing the setup of some development environments, including `.devcontainer`. ## Bug Details ### Repro Run the following command: ```bash docker build -f libs/langchain/dev.Dockerfile . ``` ### Current Behavior The docker build command fails, raising the following error: ``` ... => [langchain-dev-dependencies 4/5] COPY libs/community/ ../community/ 0.4s => ERROR [langchain-dev-dependencies 5/5] RUN poetry install --no-interaction --no-ansi --with dev,test,docs 1.1s ------ > [langchain-dev-dependencies 5/5] RUN poetry install --no-interaction --no-ansi --with dev,test,docs: #13 0.970 #13 0.970 Directory ../text-splitters does not exist ------ executor failed running [/bin/sh -c poetry install --no-interaction --no-ansi --with dev,test,docs]: exit code: 1 ``` ### Expected Behavior The `docker build` command successfully completes without the poetry error. ### Analysis The error occurs because the `text-splitters` directory is not copied into the build environment, unlike the other packages under the `libs` directory. I suspect that the `COPY` setting was overlooked since `text-splitters` was separated in a recent PR. ## Fix Add the following lines to the `libs/langchain/dev.Dockerfile`: ```dockerfile # Copy the text-splitters library for installation COPY libs/text-splitters/ ../text-splitters/ ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there,
this patch intends to support fixing GH-11, and to make progress towards GH-12. From now on, all instances 1 of SQLAlchemy model types will be created at runtime through the
ModelFactoryutility, effectively making the storage model more dynamic.By using
__table_args__ = {"keep_existing": True}on the ORM entity definitions, this seems to work well, even with multiple invocations ofCrateDBVectorSearch.from_texts()using differentcollection_nameargument values.The patch is very hard to read. Additional context/explanation: 1.
With kind regards,
Andreas.
Footnotes
There have been some details of the SA model definitions which still happened at compile-time beforehand. Now, everything concerned about establishing SA model classes is happening at runtime. This may feel weird, but is the approved way to get more flexibility. A future patch will probably leverage that flexibility by also configuring the table name based on variants. ↩ ↩2