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

Adding one to vespa passage match pages. #182

Conversation

THOR300
Copy link
Contributor

@THOR300 THOR300 commented Nov 22, 2023

Description

Updating the pydantic object for a search response that is utilised by the vespa conditional flow for a search response.
The update is to add a root validator method that increments the passage match page number by one.

Type of change

  • Bug fix
  • New feature
  • Breaking change

How Has This Been Tested?

Added some explicit tests to assert that the response objects correctly convert the index of the page to the required page number. This should help us should we upgrade pydantic etc.
The plan is once merged to deploy to staging and verify via the api.

Reviewer Checklist

  • The PR represents a single feature (small driveby fixes are also ok)
  • The PR includes tests that are sufficient for the level of risk
  • The code is sufficiently commented, particularly in hard-to-understand areas
  • Any required documentation updates have been made
  • Any TODOs added are captured in future tickets
  • No FIXMEs remain

Copy link

linear bot commented Nov 22, 2023

PDCT-549 Update Vespa passage match page numbers indexing to match OpenSearch

Please see screenshot for a visual view of the issue.

In OpenSearch the pages are index from 1 (I think), whereas in Vespa it appears we are using a 0 indexing. So page 23 in OpenSearch is page 22 in Vespa. This is obviously having an affect when we are displaying the passage highlights within the app.

Rather than me writing lots of business logic to determine which search provider we are using and adjusting the page number accordingly, could we update the vespa results to match that provided by OpenSearch on page numbers? I'm happy to revisit this later when we remove OpenSearch.

@THOR300 THOR300 requested a review from olaughter November 22, 2023 14:07
@THOR300 THOR300 marked this pull request as ready for review November 22, 2023 14:33
Copy link
Contributor

@olaughter olaughter left a comment

Choose a reason for hiding this comment

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

Good work! Great adding a test too 🙌

tests/unit/app/schemas/test_schemas.py Outdated Show resolved Hide resolved
tests/unit/app/schemas/test_schemas.py Show resolved Hide resolved
@THOR300 THOR300 merged commit b6425c7 into main Nov 22, 2023
2 checks passed
@THOR300 THOR300 deleted the feature/pdct-549-update-vespa-passage-match-page-numbers-indexing-to-match branch November 22, 2023 14:44
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.

2 participants