Ok great. Thanks for the feedback.
Random fail
I've been granted maintainership of ai_vdb_provider_opensearch so we should just decide when the best time would be to push this MR over there.
Rebased on 11.x
Thanks for your investigation in #12.
Given that, I think we should close this won't fix. Please re-open if there is an alternative approach.
Yep I think your changes are good.
I'm thinking we should get things working here, then push it back to the ai_vdb_provider_opensearch module. Having it as a sub-module while the API is experimental is going to be problematic. We want to keep releases for the search_api_opensearch module fairly stable, while this might need multiple frequent releases until it's stable.
I've reached out to @fago and @Maximillian Mikus in Slack to see if we can be added as maintainers (if you agree) of ai_vdb_provider_opensearch →
kim.pepper → made their first commit to this issue’s fork.
Should we postpone this on 🐛 SearchApiAiSearchBackend should ask configured VDB provider to supply dependencies Active or can we work around it for now?
Added the special chars to config with a default fallback.
I think this is ready to go.
smustgrave → credited kim.pepper → .
I used xdebug to work through this code locally and the test skips \Normalizer::normalize()
because class_exists('Normalizer')
returns false.
However, it successfully converts ö
to oe
in \Drupal\Component\Transliteration\PhpTransliteration::replace()
and so the test passes.
I can't really take this any further until we get some input from the transliteration maintainer (@amateescu). I'm changing the component to transliteration system for that.
Test-only pipeline did not fail as expected.
I made a naive attempt at a test. I'm not proficient any any other language besides English, so would like someone to weigh in and ensure we have the right test coverage here.
Ran the test-only pipeline to see if it fails as expected.
Thanks for the explanation and the MR. This needs a test so we can show it failing before and fixed after.
Looks like a straight forward fix.
Marking this as a feature request.
Added an Embedding object, a validator and unit test. Needs to be integrated into \Drupal\ai\Base\AiVdbProviderClientBase::validateRetrievedEmbedding() still.
kim.pepper → created an issue.
We are duplicating the methods in PluginBase
e.g. getPluginId()
and getPluginDefinition()
and the $configuration property. Why not just extend PluginBase
?
Also, reverted the key module in info.yml change.
IMO it would make sense to stick with the Search API language already well established with the Drupal community.
Committed to 1.x
Committed to 3.x and 2.x. Thanks!
Created 📌 Add a BeforeIndexCreateEvent Active for the new event.
kim.pepper → created an issue.
NW for the failing test:
Drupal\Tests\workspaces\Kernel\WorkspacesFileItemTest::testGenerateSampleValue
RuntimeException: The "field_config" entity type can only be saved in the default workspace.
kim.pepper → created an issue.
We have moved this to it's own project https://www.drupal.org/project/search_api_opensearch_semantic → with the latest from MR !107.
Please continue any work over there.
Marking this issue as Fixed to provide contribution credits.
As per @jonathan1055 comment in #75, 🐛 FileItem::generateSampleValue() should generate files with the actual allowed extensions Needs work I think we should go back to @dww's patch in comment #36 🐛 FileItem::generateSampleValue() should generate files with the actual allowed extensions Needs work .
Created an MR for that.
kim.pepper → made their first commit to this issue’s fork.
Re-ran the tests and they are back to green.
Tests are failing and comments in #9 need to be addressed. See https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → for details on what makes an issue RTBC.
Would be good to have an alternative to \Drupal::service() that takes a class name so we can do this:
/**
* @template T of object
* @param class-string<T> $className
* @return T
*/
function createInstance(string $className): object {}
...and have phpstan do the validation.
This was just a reroll so leaving at RTBC
Rebased on 11.x
We will need to update the change record once 🐛 Deprecated file_system_settings_submit() calls settingsSubmit() on wrong class Active is in.
kim.pepper → created an issue.
📌 Move file form hooks to FileFormHooks class Active landed.
Updating the title to better reflect the direction.
I added a vector document and an interface for a vector store.
Updating to include the following params which are also unused:
protected EventDispatcherInterface $eventDispatcher
protected MessengerInterface $messenger
kim.pepper → created an issue.
kim.pepper → created an issue.
kim.pepper → created an issue.
Reviewing as part of Bug Smash Initiative. As per the comment in #25 marking this as PMNMI.
kim.pepper → created an issue.
kim.pepper → created an issue.
kim.pepper → created an issue.
marcus_johansson → credited kim.pepper → .
kim.pepper → created an issue.
We might want a followup to make a FiltersInterface
instead of mixed?
kim.pepper → created an issue.
Created a MR that splits out this dependency.
Do we need a meta issue to track these tasks?
Why not just split out the Search API dependent code to the ai_search
submodule, rather than conditional logic?
There also seems to be some entanglement with both:
\Drupal\ai\AiVdbProviderInterface::querySearch()
and\Drupal\ai\AiVdbProviderInterface::vectorSearch()
depending on the output of \Drupal\ai_search\AiVdbProviderSearchApiInterface::prepareFilters()
in the ai_search
submodule.
Should these be moved to the ai_search
submodule too?
Added a draft MR with the basic plugin. We need to implement all the stub methods and save and retrieve connector configuration.
Also a lot of this is duplicated in \Drupal\search_api_opensearch\Plugin\search_api\backend\OpenSearchBackend
so maybe we need a trait?
As this is only an issue on unsupported versions I think we can safely close.
Fixed the failing test by handling the exception. Not sure if this is the right place to handle it.
Added 📌 Move file form hooks to FileFormHooks class Active for the hooks cleanup mentioned above.
Added a todo for replacing the static \Drupal\file\Hook\FileFormHooks::settingsSubmit() with a non-static once 📌 Use CallableResolver for form callbacks Active is in.
kim.pepper → created an issue.
Yes. I discussed this with @fago in slack and we decided to move it here. We can re-use the connection plugin logic and existing search api implementation.
kim.pepper → made their first commit to this issue’s fork.
Yes CallableResolver
supports the service notation.
kim.pepper → created an issue.
We need a IS and title update
Looks like this should be re-opened based on the comments above.
Awesome thanks! When can we expect this in a release?
We should actually do this in search_api_opensearch. Created a new issue 📌 Add support for AI module VDB Provider Active
Closing as a duplicate of 🐛 Check for new files when validating size Needs work
@mutasim al-shoura You had commented on that issue already. Not sure why you opened a new issue?
Agree with @berdir. Using CallableResolver
gives us more options and proper validation of callables.
kim.pepper → created an issue.
kim.pepper → created an issue.
kim.pepper → created an issue.
kim.pepper → created an issue.
This caused a BC break in 🐛 Make compatible with 1.1.x series of the AI module Active What's the BC policy for this module? I assumed we can't just add params to interfaces in minor releases?
Looks like a BC breaking Drupal\search_api\Query\QueryInterface
param was added to \Drupal\ai\AiVdbProviderInterface::vectorSearch
in
✨
Pass metric type to vector search function
Active
.
We don't seem to need it?
I think it could provide some nice interfaces and models for us to use.