Introduce neural highlighter framework #1193

junqiu-lei · 2025-02-19T22:38:32Z

Description

This PR introduces the Neural Highlighter framework from OpenSearch core Highlighter, enhancing the semantic highlighting capabilities. Key changes include:

Added NeuralHighlighter with core functionality.
Updated NeuralSearch and NeuralKNNQuery for integration.
Added unit test for NeuralHighlighter (IT will be added when all functionality is ready).

PR opensearch-project/ml-commons#3600 adds the functionality in ml-commons to use sentence highlighting model.

Related Issues

Part of #1182

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Junqiu Lei <junqiu@amazon.com>

heemin32 · 2025-02-21T00:07:13Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+    public static final String NAME = "neural";
+    private static final String MODEL_ID_FIELD = "model_id";
+
+    private static MLCommonsClientAccessor mlCommonsClient;


This static pattern is not ideal for unit test. Can we receive accessor thought constructor?

heemin32 · 2025-02-21T00:09:06Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+
+    @Override
+    public boolean canHighlight(MappedFieldType fieldType) {
+        // TODO: Implement actual condition check in subsequent PR


Could be better to restrict it to TEXT field type and expand it later in subsequent PR.

heemin32 · 2025-02-21T00:11:29Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+
+            Map<String, Object> options = fieldContext.field.fieldOptions().options();
+            String modelId = getModelId(options);
+            log.info("Using model ID: {}", modelId); // Will be replaced with actual model loading logic


This log will be flooded and obscure other important logs. Let't move it to debug level.

I was trying to add a temp placeholder here, since client api function was planed in another PR.

Please add javadoc and a comment of all the important steps this method is trying to do.

@vibrantvarun sure

heemin32 · 2025-02-21T00:12:47Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+            log.info("Using model ID: {}", modelId); // Will be replaced with actual model loading logic
+            log.info("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic
+
+            // TODO: Implement actual highlighting logic in subsequent PR


With all these TODO, it seems like this PR should not be merged to main now.
Let me stop here and review it when the code is fully completed.

Got it. My original purpose was trying to make the PR more clear by different component(highlighter framework, ml client, text processing logic...). I could also combine all code in one big PR.

martin-gaievski · 2025-02-21T00:23:54Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+
+    private String formatHighlight(String text) {
+        // TODO: Implement user provided format options in subsequent PR
+        return "<em>" + text + "</em>";


use String format for safer concatenation

martin-gaievski · 2025-02-21T00:25:17Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+            }
+        }
+
+        return query.toString().replaceAll("\\w+:", "").replaceAll("\\s+", " ").trim();


please add comment describing what you are trying to extract from string here

martin-gaievski · 2025-02-21T00:33:54Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+            // For now, return a basic highlight of the field text
+            Text[] fragments = new Text[] { new Text(formatHighlight(fieldText)) };
+            return new HighlightField(fieldContext.fieldName, fragments);
+        } catch (Exception e) {


is it possible to narrow cases when we can have exceptions? Having global level try/catch is not that great for performance on search path

I see, let me try to adjust to narrow exception catch cases.

martin-gaievski · 2025-02-21T00:38:55Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+    }
+
+    private String getFieldText(FieldHighlightContext fieldContext) {
+        Object value = fieldContext.hitContext.sourceLookup().extractValue(fieldContext.fieldName, null);


isn't this second argument a fallback in case requested key is missing? If yes can't we return empty string here and avoid the null check on a line below?

vibrantvarun · 2025-02-21T17:49:42Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+
+            Map<String, Object> options = fieldContext.field.fieldOptions().options();
+            String modelId = getModelId(options);
+            log.info("Using model ID: {}", modelId); // Will be replaced with actual model loading logic


Please add javadoc and a comment of all the important steps this method is trying to do.

vibrantvarun · 2025-02-21T17:50:21Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+            Map<String, Object> options = fieldContext.field.fieldOptions().options();
+            String modelId = getModelId(options);
+            log.info("Using model ID: {}", modelId); // Will be replaced with actual model loading logic
+            log.info("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic


Suggested change

log.info("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic

log.debug("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic

Curious why do we need this log this?

See the comment along with this line code

vibrantvarun · 2025-02-21T17:51:32Z

src/main/java/org/opensearch/neuralsearch/highlight/NeuralHighlighter.java

+
+    private String extractOriginalQuery(Query query) {
+        if (query instanceof NeuralKNNQuery neuralQuery) {
+            String originalText = neuralQuery.getOriginalQueryText();


Suggested change

String originalText = neuralQuery.getOriginalQueryText();

String originalQuery = neuralQuery.getOriginalQueryText();

vibrantvarun · 2025-02-21T17:52:24Z

src/main/java/org/opensearch/neuralsearch/query/NeuralKNNQuery.java

        this.knnQuery = knnQuery;
+        this.originalQueryText = originalQueryText;


Shall we add BWC test to validate that it does not break anything in bwc environment?

Sure, will have a check on bwc side

vibrantvarun · 2025-02-21T17:53:06Z

src/main/java/org/opensearch/neuralsearch/query/NeuralKNNQueryBuilder.java

@@ -107,6 +109,11 @@ public Builder rescoreContext(RescoreContext rescoreContext) {
            return this;
        }

+        public Builder originalQueryText(String originalQueryText) {


Please add javadoc. We already have an issue for reducing the warning logs #1095

If possible please add the javadoc on all public methods of this class.

junqiu-lei · 2025-03-03T19:08:34Z

For reference in this PR, in ml-commons we have PR for sentence highlighting model support :
opensearch-project/ml-commons#3600

junqiu-lei added Features Introduces a new unit of functionality that satisfies a requirement v3.0.0 v3.0.0 labels Feb 19, 2025

junqiu-lei self-assigned this Feb 19, 2025

junqiu-lei requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, martin-gaievski, sean-zheng-amazon, model-collapse, zane-neo, vibrantvarun, zhichao-aws, yuye-aws and minalsha as code owners February 19, 2025 22:38

junqiu-lei mentioned this pull request Feb 19, 2025

Neural Highlighting Project Tasks Tracker #1182

Open

6 tasks

junqiu-lei added the neural-search label Feb 19, 2025

Introduce neural highlighter framework

9704f6c

Signed-off-by: Junqiu Lei <junqiu@amazon.com>

junqiu-lei force-pushed the highlighter-framework branch from 1bc7505 to 9704f6c Compare February 19, 2025 22:41

q-andy approved these changes Feb 20, 2025

View reviewed changes

heemin32 reviewed Feb 21, 2025

View reviewed changes

martin-gaievski reviewed Feb 21, 2025

View reviewed changes

vibrantvarun reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce neural highlighter framework #1193

Introduce neural highlighter framework #1193

junqiu-lei commented Feb 19, 2025 •

edited

Loading

heemin32 Feb 21, 2025

junqiu-lei Feb 21, 2025

heemin32 Feb 21, 2025

junqiu-lei Feb 21, 2025

heemin32 Feb 21, 2025

junqiu-lei Feb 21, 2025

vibrantvarun Feb 21, 2025

junqiu-lei Feb 21, 2025

heemin32 Feb 21, 2025

junqiu-lei Feb 21, 2025

martin-gaievski Feb 21, 2025

martin-gaievski Feb 21, 2025

martin-gaievski Feb 21, 2025

junqiu-lei Feb 21, 2025

martin-gaievski Feb 21, 2025

vibrantvarun Feb 21, 2025

vibrantvarun Feb 21, 2025

vibrantvarun Feb 21, 2025

junqiu-lei Feb 21, 2025

vibrantvarun Feb 21, 2025

vibrantvarun Feb 21, 2025

junqiu-lei Feb 21, 2025

vibrantvarun Feb 21, 2025

vibrantvarun Feb 21, 2025

junqiu-lei commented Mar 3, 2025 •

edited

Loading

	log.info("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic
	log.debug("Using ML client: {}", mlCommonsClient); // Will be replaced with actual model loading logic

	String originalText = neuralQuery.getOriginalQueryText();
	String originalQuery = neuralQuery.getOriginalQueryText();

		this.knnQuery = knnQuery;
		this.originalQueryText = originalQueryText;

Introduce neural highlighter framework #1193

Are you sure you want to change the base?

Introduce neural highlighter framework #1193

Conversation

junqiu-lei commented Feb 19, 2025 • edited Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junqiu-lei commented Mar 3, 2025 • edited Loading

junqiu-lei commented Feb 19, 2025 •

edited

Loading

junqiu-lei commented Mar 3, 2025 •

edited

Loading