🏄‍♂️🇦🇺Sydney, Australia
Account created on 24 September 2008, almost 17 years ago
  • Co-Founder and Technical Director at PreviousNext 
  • Co-Founder and Technical Director at Skpr 
#

Merge Requests

More

Recent comments

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Ok great. Thanks for the feedback.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Random fail

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Rebased on 11.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Yep I think your changes are good.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper made their first commit to this issue’s fork.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Should we postpone this on 🐛 SearchApiAiSearchBackend should ask configured VDB provider to supply dependencies Active or can we work around it for now?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Added the special chars to config with a default fallback.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

I think this is ready to go.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Thanks for the explanation and the MR. This needs a test so we can show it failing before and fixed after.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Looks like a straight forward fix.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Marking this as a feature request.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Added an Embedding object, a validator and unit test. Needs to be integrated into \Drupal\ai\Base\AiVdbProviderClientBase::validateRetrievedEmbedding() still.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

IMO it would make sense to stick with the Search API language already well established with the Drupal community.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 1.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 3.x and 2.x. Thanks!

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Created 📌 Add a BeforeIndexCreateEvent Active for the new event.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper made their first commit to this issue’s fork.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Re-ran the tests and they are back to green.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

This was just a reroll so leaving at RTBC

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Rebased on 11.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Updating the title to better reflect the direction.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

I added a vector document and an interface for a vector store.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Updating to include the following params which are also unused:

  • protected EventDispatcherInterface $eventDispatcher
  • protected MessengerInterface $messenger
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Reviewing as part of Bug Smash Initiative. As per the comment in #25 marking this as PMNMI.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

We might want a followup to make a FiltersInterface instead of mixed?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Created a MR that splits out this dependency.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Do we need a meta issue to track these tasks?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

As this is only an issue on unsupported versions I think we can safely close.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Fixed the failing test by handling the exception. Not sure if this is the right place to handle it.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Added 📌 Move file form hooks to FileFormHooks class Active for the hooks cleanup mentioned above.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper made their first commit to this issue’s fork.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Yes CallableResolver supports the service notation.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

We need a IS and title update

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Looks like this should be re-opened based on the comments above.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Awesome thanks! When can we expect this in a release?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

We should actually do this in search_api_opensearch. Created a new issue 📌 Add support for AI module VDB Provider Active

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Agree with @berdir. Using CallableResolver gives us more options and proper validation of callables.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

I think it could provide some nice interfaces and models for us to use.

Production build 0.71.5 2024