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

feat: Index key generation related fixes #289

Merged
merged 14 commits into from
May 3, 2024

Conversation

chandrasekharan-zipstack
Copy link
Contributor

@chandrasekharan-zipstack chandrasekharan-zipstack commented Apr 25, 2024

What

  • Added migration to delete incorrect index keys
  • Updated index key generation in prompt service
  • MINOR: Corrected the /document-index request to include a trailing slash and avoid an unnecessary redirect
  • Version and lockfile updates for tools and other services to use SDK 0.24.0

Why

  • More context in SDK PR

Can this PR break any existing features. If yes please list of possible items. If no please explain why. (PS: Admins do not merge the PR without this section filled)

  • Yes, its related to an SDK PR. Since there's an update in the index key generated - all old index keys would be invalid. In order to perform a clean up - a migration has been added

Database Migrations

  • Simple migration - clearing all old index key records only if they matched the regex from the old logic. This would ensure no new records get deleted unnecessarily if migrations are re-run

Env Config

Relevant Docs

Related Issues or PRs

Dependencies Versions

Notes on Testing

  • Explicitly checked DB before and after migrations
  • Also tried to update the adapters after it was indexed once to ensure it was indexed again

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

Copy link
Contributor

@hari-kuriakose hari-kuriakose left a comment

Choose a reason for hiding this comment

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

@chandrasekharan-zipstack Data migration looks ok.

However let's test the migration once before we merge this in.

@chandrasekharan-zipstack
Copy link
Contributor Author

chandrasekharan-zipstack commented Apr 30, 2024

@hari-kuriakose regarding this - in the current system as well we don't remove the stale entries. I thought we could handle all of it together with a separate task to delete these vector DB entries when indexes are invalidated.
But now that you mention it we might end up with stray records in the vector DB for these keys that will be deleted - we might have to loop over all the vector DB instances which was used to index and then invoke some delete method to do this. In the interest of time, we decided to ignore these entries alone
CC: @nehabagdia @tahierhussain

@chandrasekharan-zipstack chandrasekharan-zipstack changed the title WIP: Index key generation related fixes feat: Index key generation related fixes May 2, 2024
@chandrasekharan-zipstack chandrasekharan-zipstack marked this pull request as ready for review May 2, 2024 07:08
Copy link

sonarcloud bot commented May 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@nehabagdia nehabagdia merged commit 8db05bf into main May 3, 2024
4 checks passed
@nehabagdia nehabagdia deleted the feat/index-key-generation-fix branch May 3, 2024 10:05
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

5 participants