Skip to content

Use 613 must should drop#20

Merged
JPrevost merged 4 commits into
mainfrom
use-613-must-should-drop
Jun 12, 2026
Merged

Use 613 must should drop#20
JPrevost merged 4 commits into
mainfrom
use-613-must-should-drop

Conversation

@JPrevost

@JPrevost JPrevost commented Jun 11, 2026

Copy link
Copy Markdown
Member

Note: if it isn't clear how to see this in action even with the notes, I'd be happy to do a quick zoom.

Why are these changes being introduced:

Our current semantic query will return 10,000+ results for most queries
of more than a couple words. This is because all the rank_feature
queries were created as should. This means it’s basically an OR query.

The top results are good, but at some point the results become quite
irrelevant -- for instance showing every record with the word and in
it.

Relevant ticket(s):

How does this address that need:

  • Most semantically relevant words moved to must block in query.
  • Less relevant move to should.
  • Least relevant words dropped for longer queries.

This was done by introducing adjustable threshholds that control our
implementation.

How can a reviewer manually see the effects of these changes?

Option 1: In dev1, you can go to the live lambda and run queries there to generate the structure you can then use in an OpenSearch Dashboard to see the difference between this code and similar output in the prod tier lambda.

Option 2: You can run these commands locally:

  • make console
  • from lambdas import tokenizer_handler
  • words = "The quick brown fox jumped over the lazy dog."
  • tokenizer_handler.lambda_handler({"query": words}, {})

To see the configurable options, you can add parameters like:

  • tokenizer_handler.lambda_handler({"query": words, "short_query_max_tokens": "1", "drop_boost_threshold": ".5", "max_boost_threshold": ".95"}, {})

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO (the contract remains the same, the results will be quite different though)

JPrevost added 2 commits June 10, 2026 09:53
Why are these changes being introduced:

Our current semantic query will return 10,000+ results for most queries
of more than a couple words. This is because all the rank_feature
queries were created as should. This means it’s basically an OR query.

The top results are good, but at some point the results become quite
irrelevant -- for instance showing every record with the word `and` in
it.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/USE-613

How does this address that need:

* Most semantically relevant words moved to must block in query.
* Less relevant move to should.
* Least relevant words dropped for longer queries.

This was done by introducing adjustable threshholds that control our
implementation.
Why are these changes being introduced:

* Having to deploy the lambda to test different thresholds was going
  to be annoying.
* This allows us to test different thresholds without having to deploy
  the lambda.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/USE-613

How does this address that need:

* Allows overriding the default thresholds with values in the
  event object.

Document any side effects to this change:

* Once we determine the best thresholds, we should update the default
  values and consider removing the option to override them entirely.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR changes how the tokenizer Lambda constructs OpenSearch bool queries so that higher-weight tokens become required (must), lower-weight tokens remain optional (should), and very-low-weight tokens can be dropped for longer queries—reducing overly-broad result sets (USE-613).

Changes:

  • Introduces configurable thresholds to split token rank_feature clauses into must vs should, and optionally drop very-low-weight tokens for longer queries.
  • Refactors query construction into a dedicated _build_opensearch_query() helper.
  • Expands unit test coverage for query construction behavior and threshold overrides.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
lambdas/tokenizer_handler.py Adds thresholds + _build_opensearch_query() and uses it from the Lambda handler, with per-invocation override support.
tests/test_tokenizer_handler.py Updates existing handler tests and adds focused unit tests for _build_opensearch_query() behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lambdas/tokenizer_handler.py Outdated
Comment thread lambdas/tokenizer_handler.py Outdated
Comment thread tests/test_tokenizer_handler.py Outdated
Comment thread tests/test_tokenizer_handler.py Outdated
Comment thread tests/test_tokenizer_handler.py
@qltysh

qltysh Bot commented Jun 11, 2026

Copy link
Copy Markdown

1 new issue

Tool Category Rule Count
radarlint-python Lint Remove the unused local variable "ValueError". 1

short_raw = event.get("short_query_max_tokens", SHORT_QUERY_MAX_TOKENS)
try:
must_boost_threshold = float(must_raw)
except TypeError, ValueError:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove the unused local variable "ValueError". [radarlint-python:python:S1481]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread lambdas/tokenizer_handler.py
Comment thread lambdas/tokenizer_handler.py
Comment thread lambdas/tokenizer_handler.py
@matt-bernhardt matt-bernhardt self-assigned this Jun 11, 2026

@matt-bernhardt matt-bernhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, and I'm looking forward to seeing how these queries result in different results moving forward. I suspect that we intend to just use the default values to start with, without worrying about trying to tune behavior immediately?

I have a for-clarity question about how the short-query threshold actually functions, and there are a few tests that I think might be differently structured - but none of these points are blocking a merge if you'd like to move forward as-is.

Comment thread tests/test_tokenizer_handler.py Outdated
Comment thread tests/test_tokenizer_handler.py Outdated
short_query_max_tokens = SHORT_QUERY_MAX_TOKENS
must_boost_threshold = max(0.0, min(1.0, must_boost_threshold))
drop_boost_threshold = max(0.0, min(1.0, drop_boost_threshold))
short_query_max_tokens = max(0, short_query_max_tokens)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is probably a very minor point, but I want to understand the impact of the short-query threshold. I see that we're clamping the minimum value to 0, but in effect the lowest meaningful threshold is 2, I think? Passing a short threshold below 2 has the same effect as 2, because a threshold of 0 or 1 will have no effect (a query with 1 token will never be shortened, because it will have the max token weight, but a query with 2 tokens might be shortened).

I'm happy with the feature as designed, I think being able to strip out a single token using this threshold would be a very bad design choice - so there may not be a change to make here. I'm asking to make sure that I'm interpreting the behavior I see in the console correctly:

In [11]: tokenizer_handler.lambda_handler({"query": "the dominant"}, {})
Out[11]: 
{'query': {'bool': {'must': [{'rank_feature': {'field': 'embedding_full_record.dominant',
      'boost': 6.694228649139404}}],
   'should': [{'rank_feature': {'field': 'embedding_full_record.the',
      'boost': 0.1353016495704651}}]}}}

In [12]: tokenizer_handler.lambda_handler({"query": "the dominant", "short_query_max_tokens": 0}, {})
Out[12]: 
{'query': {'bool': {'must': [{'rank_feature': {'field': 'embedding_full_record.dominant',
      'boost': 6.694228649139404}}]}}}

In [13]: tokenizer_handler.lambda_handler({"query": "the", "short_query_max_tokens": 0}, {})
Out[13]: 
{'query': {'bool': {'must': [{'rank_feature': {'field': 'embedding_full_record.the',
      'boost': 0.1353016495704651}}]}}}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think you do understand it well and the code could probably use some tune ups to put in better clamps, but I may not at this time. I'll give it some thought.

@JPrevost JPrevost merged commit 6c88436 into main Jun 12, 2026
10 checks passed
@JPrevost JPrevost deleted the use-613-must-should-drop branch June 12, 2026 12:22
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.

3 participants