[Store] Add commands to setup/drop a store #335
Conversation
Guikingone
requested review from
chr-hertel,
Nyholm and
OskarStark
as code owners
carsonbot
added
Status: Needs Review
Feature
carsonbot
changed the title
Guikingone
changed the title
Guikingone
force-pushed
the
store/commands
branch
from
41b5cd7 to
3f6ef49
Compare
OskarStark
changed the title
Guikingone
force-pushed
the
store/commands
branch
2 times, most recently
from
1ef933d to
30c5f2f
Compare
| $stores = array_keys($builder->findTaggedServiceIds('ai.store')); | ||
| if (1 === \count($stores)) { | ||
| $builder->setAlias(StoreInterface::class, reset($stores)); |
There was a problem hiding this comment.
unexpected to see this being removed - was it intentional?
There was a problem hiding this comment.
Yes, as the aliases are defined for each store, I'm not sure if we need this alias anymore 🤔
There was a problem hiding this comment.
please revert this, see #355
It should still be possible to just use StoreInterface $store if only store is configured - which is basically quite common, i'd say
There was a problem hiding this comment.
BTW, regarding #355, as I brought the issue, do you want me to work on the fix?
no no, that the right decision from my point of view 👍 |
|
Alright, let's talk about functional testing here - that's important to me with that amount of bridges we can't go with manual. My first idea was to bring in simple store examples, that would setup, add, query and drop. but that would be super similar to the rag ones - well without the agent tho. |
|
What about improving the rag examples by adding the Maybe just before returning the content to the user? Don't know if adding examples with commands brings any benefits to the user or we're talking about single-file commands that we can call in the existing examples 🤔 |
|
I would keep the examples, but what about using setup and store in CI to see if they are working? Could be a follow up PR imho |
|
Docs are missing |
Guikingone
force-pushed
the
store/commands
branch
2 times, most recently
from
853094b to
c1bcf91
Compare
|
If we could shift it to GitHub actions, that would be fine as well - and it's true, that there is little benefit to users with examples only setup-add-query-drop, true. We already skipped it in #328 - so we should bring it in now. |
Guikingone
force-pushed
the
store/commands
branch
3 times, most recently
from
f00ae61 to
bdd97ab
Compare
|
I was wondering, shouldn't we use the We can build a container then make the commands accessible but we already have the |
| SYMFONY_REQUIRE: ${{ matrix.symfony-version || '>=6.4' }} | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v5 |
The demo currently has only one store - and I like that simplicity there. But I get your point. So I see two options, test the functional integration only on service level with a PHPUnit testcase neglecting the command level, or bring in a small example script with bootstrapping a console application and registering the commands there. i'm not sure we need a separate container image but go with the setup-php base. but maybe i'm missing something here. |
|
Let's add a small console + commands then 👍🏻 |
Guikingone
force-pushed
the
store/commands
branch
from
a0cd1d4 to
481e8f2
Compare
Hi 👋🏻
As discussed in #330, here's the commands required to setup/drop the stores, by default, I moved the commands in the
Storecomponent but wasn't sure about it.