Skip to content
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

PoC for Vectara integration #826

Merged
merged 4 commits into from Sep 11, 2023
Merged

PoC for Vectara integration #826

merged 4 commits into from Sep 11, 2023

Conversation

mkr
Copy link
Contributor

@mkr mkr commented Sep 11, 2023

This brings in a simple integration into the Vectara search engine.

This requires the respective PR in splainer-search to be merged.

Currently, this is mainly supporting sending queries and getting documents back from Vectara.

Some limitations:

  • There is currently no "explain" output.
  • Vectara is separating responses from documents (where there can be multiple response for a single document). We are currently extracting the documents from the response.
  • probably more...

@@ -55,7 +55,7 @@ angular.module('QuepidApp')
var docIds = Object.keys(docsToFetch);
var resolver = docResolverSvc.createResolver(docIds, settings, 15);

if ( docIds.length > 0 ) {
if ( docIds.length > 0 && settings.searchEngine !== 'vectara') { // 'vectara' does not support doc lookup by ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine... And pionts to, in the future, we need some sort of "capablities" so you can be like settings.searchEngine.supportDocLookup instead ;-)

@@ -69,7 +71,7 @@ angular.module('QuepidApp')
});

function createSearcherFromSettings(passedInSettings, queryText, query) {
let args = angular.copy(passedInSettings.selectedTry.args);
let args = angular.copy(passedInSettings.selectedTry.args) || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this? I have only seen this as needed when something blows up.... I see the args parameter in try.rb now supports vectara! so this may not be needed.

if (args['fq'] === undefined) {
args['fq'] = [];
}
args['fq'].push(query.filterToRatings(passedInSettings));
} else if (passedInSettings.searchEngine === 'vectara') {
// currently doc id filtering frequently produces 0 results
// args['query'] = args['query'].map(function addFilter(query) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially deleteable code? I like the explicit else if for reminding us that vectara doc id filtering... Again, maybe soem day we have capablities to decide this stuff ;-).

@@ -83,6 +81,7 @@ angular.module('QuepidApp')

escapeQuery: true,
apiMethod: 'POST',
customHeaders: '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add it here if we remove it above? In some places we do a check for either for existence of customheaders or an actual check of if it is '', are we maybe missing that someplace?

customHeaders: '',
fieldSpec: 'id:id',
idField: 'id',
titleField: 'title',
Copy link
Member

@epugh epugh Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the titleField and idField... are those values "standard" in Vectara land? Do we/they want to host a demo Vectara that we can document and use in the wizard, kind of like the TMDB dataset? And add to the https://github.com/o19s/quepid/wiki/Troubleshooting-Opensearch-and-Quepid#demo-opensearch-indexes-with-the-tmdb-dataset type page for Vectara???

</div>
</div>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! this looks like some nasty html nesting bug!

@epugh epugh temporarily deployed to quepid-pr-826 September 11, 2023 21:23 Inactive
@epugh epugh temporarily deployed to quepid-pr-826 September 11, 2023 21:30 Inactive
@epugh epugh temporarily deployed to quepid-pr-826 September 11, 2023 21:36 Inactive
@epugh epugh temporarily deployed to quepid-pr-826 September 11, 2023 21:59 Inactive
@epugh epugh merged commit 3c45ade into main Sep 11, 2023
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants