Skip to content

Clean opensearch final #2072

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SouthKoreaLN
Copy link
Contributor

@SouthKoreaLN SouthKoreaLN commented Apr 6, 2025

Cfr #2043.

Description

This PR upgrades OpenSearch and OpenSearch Dashboards from version 2.17.0 to 2.19.0 in docker-compose.yml, which introduces proper pagination support for hybrid search queries.

Additionally, it sets pagination_depth: 50 in the hybrid search query (api/resolvers/search.js) to enable consistent and correct pagination when using the "More" button in search results.

Screenshots

N/A

Additional Context

The fix relies on OpenSearch 2.19.0's new hybrid pagination support, documented here:
https://opensearch.org/docs/latest/vector-search/ai-search/hybrid-search/pagination/

No other files or functionality are touched.

Checklist

Are your changes backwards compatible? Please answer below:
Yes — the upgrade and query change are backwards compatible with existing usage.

On a scale of 1–10 how well and how have you QA'd this change and any features it might affect? Please answer below:
3: the change was tested in the sndev Docker environment, where hybrid pagination was already working as expected. The version bump to OpenSearch 2.19.0 and the addition of pagination_depth were applied, but full end-to-end validation of the bug fix was limited due to the issue not being reproducible in this environment.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
N/A — backend change only

Did you introduce any new environment variables? If so, call them out explicitly here:
No new environment variables

Resolves #2043.

@ed-kung
Copy link
Contributor

ed-kung commented Apr 6, 2025

the change was tested in the sndev Docker environment, where hybrid pagination was already working as expected. The version bump to OpenSearch 2.19.0 and the addition of pagination_depth were applied, but full end-to-end validation of the bug fix was limited due to the issue not being reproducible in this environment.

Just a quick note: Unless you had set up the semantic search using these steps, your dev instance probably wasn't using semantic search. (It's not set up by default.) That could be why you couldn't reproduce the bug.

@SouthKoreaLN
Copy link
Contributor Author

the change was tested in the sndev Docker environment, where hybrid pagination was already working as expected. The version bump to OpenSearch 2.19.0 and the addition of pagination_depth were applied, but full end-to-end validation of the bug fix was limited due to the issue not being reproducible in this environment.

Just a quick note: Unless you had set up the semantic search using these steps, your dev instance probably wasn't using semantic search. (It's not set up by default.) That could be why you couldn't reproduce the bug.

I should probably test it then. Thanks for the note. Will revert back to draft for this PR.

@SouthKoreaLN SouthKoreaLN marked this pull request as draft April 6, 2025 22:04
@ed-kung
Copy link
Contributor

ed-kung commented Apr 6, 2025

I have an open PR (#2070) to automate parts of the semantic search setup. Hopefully that makes setting it up less painful than it used to be.

@SouthKoreaLN
Copy link
Contributor Author

I have an open PR (#2070) to automate parts of the semantic search setup. Hopefully that makes setting it up less painful than it used to be.

I kinda picked this issue at random. I didn't notice this fits in a larger set of issues that are related to work you've been doing. Feel free to propose another PR that solves the same issue, especially if you notice that the changes I've proposed don't work once testing it with proper semantic search.

@ed-kung
Copy link
Contributor

ed-kung commented Apr 8, 2025

#2070 is now merged. If you merge master into this PR i'd be happy to test it for you.

BTW, glad to see you contributing to SN again :)

@SouthKoreaLN
Copy link
Contributor Author

I just merged master into this branch. Thanks for checking.

BTW, glad to see you contributing to SN again :)

I got screwed on what was supposed to be a sure permanent position recently. The jury was skewed 6 to 1 in terms of who was working in the other candidate's field and my field, so it was a lost battle from the start. There are other opportunities ahead, but I needed a bit of a change of focus. Contributing to SN is perfect for that.

@ed-kung
Copy link
Contributor

ed-kung commented Apr 8, 2025

Ah, sorry to hear that! How do permanent positions work in Korea? Perhaps we can talk about it on SN.

Anyway, I tested this and I think it works to fix pagination! You should mark it ready for review again.

@SouthKoreaLN SouthKoreaLN marked this pull request as ready for review April 8, 2025 05:00
@huumn
Copy link
Member

huumn commented Apr 8, 2025

We are stuck on 2.17 in prod until AWS supports 2.19. So I'm going to label this a blocked for the time being.

@huumn huumn added the blocked label Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination broken in search?
3 participants