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.

Sign up for GitHub

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

[Releases 2.13] Output hybrid error message in marqo logs #1108

Open
wants to merge 1 commit into
base: releases/2.13
Choose a base branch
from
+88 −0
Open

[Releases 2.13] Output hybrid error message in marqo logs #1108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions vespa/src/main/java/ai/marqo/search/HybridSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.yahoo.search.Query;
import com.yahoo.search.Result;
import com.yahoo.search.Searcher;
import com.yahoo.search.result.ErrorMessage;
import com.yahoo.search.result.Hit;
import com.yahoo.search.result.HitGroup;
import com.yahoo.search.searchchain.AsyncExecution;
Expand Down Expand Up @@ -105,6 +106,8 @@ public Result search(Query query, Execution execution) {
+ e.toString());
}

raiseErrorIfPresent(resultLexical, resultTensor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering if we should handle errors differently after reading Vespa docs. Instead of raising Runtime exception everywhere (which cause Vespa to always return 500). We should probably leverage HitGroup().addError method which is suggested in this doc.

For this specific scenario, we are collecting errors from both lexical result and tensor result. So we can do this:

HitGroup combinedErrors = new HitGroup();
combinedErrors.addErrorsFrom(resultTensor.hits());
combinedErrors.addErrorsFrom(resultLexical.hits());
if (combinedErrors.getError() != null) {
      return new Result(query, combinedErrors);
}

In this way, you should be able to collect all the errors from both result and allow search chain to handle the error properly, instead of just concatenating the first error found in each result.


logIfVerbose(
"LEXICAL RESULTS: "
+ resultLexical.toString()
Expand Down Expand Up @@ -284,6 +287,24 @@ HitGroup rrf(
return result;
}

void raiseErrorIfPresent(Result resultLexical, Result resultTensor) {
// Raise error if either result list has an error. Make sure error messages are combined
String tensorOrLexicalErrors = "";
ErrorMessage tensorError = resultTensor.hits().getError();
if (tensorError != null) {
tensorOrLexicalErrors += "Error in TENSOR search in RRF: " + tensorError + "\n";
}

ErrorMessage lexicalError = resultLexical.hits().getError();
if (lexicalError != null) {
tensorOrLexicalErrors += "Error in LEXICAL search in RRF: " + lexicalError;
}

if (!tensorOrLexicalErrors.isEmpty()) {
throw new RuntimeException(tensorOrLexicalErrors);
}
}

/**
* Extracts mapped Tensor Address from cell then adds it as key to rank features, with cell value as the value.
* @param cell
Expand Down
67 changes: 67 additions & 0 deletions vespa/src/test/java/ai/marqo/search/HybridSearcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.yahoo.component.chain.Chain;
import com.yahoo.search.*;
import com.yahoo.search.query.ranking.RankFeatures;
import com.yahoo.search.result.ErrorMessage;
import com.yahoo.search.result.Hit;
import com.yahoo.search.result.HitGroup;
import com.yahoo.search.searchchain.*;
Expand Down Expand Up @@ -379,4 +380,70 @@ private static Query getHybridQuery(
.put("query(marqo__fields_to_rank_tensor)", fieldsToRankTensor);
return query;
}

@Nested
class RaiseErrorIfPresentTest {
@Test
void shouldRaiseErrorIfLexicalResultHasError() {
Result resultLexical =
new Result(
new Query(),
ErrorMessage.createInternalServerError("Example lexical error"));
Result resultTensor = new Result(new Query());
RuntimeException exception =
assertThrows(
RuntimeException.class,
() -> {
hybridSearcher.raiseErrorIfPresent(resultLexical, resultTensor);
});
assertThat(exception.getMessage()).contains("Error in LEXICAL search in RRF:");
assertThat(exception.getMessage()).contains("Example lexical error");
}

@Test
void shouldRaiseErrorIfTensorResultHasError() {
Result resultLexical = new Result(new Query());
Result resultTensor =
new Result(
new Query(),
ErrorMessage.createInternalServerError("Example tensor error"));
RuntimeException exception =
assertThrows(
RuntimeException.class,
() -> {
hybridSearcher.raiseErrorIfPresent(resultLexical, resultTensor);
});
assertThat(exception.getMessage()).contains("Error in TENSOR search in RRF:");
assertThat(exception.getMessage()).contains("Example tensor error");
}

@Test
void shouldRaiseErrorIfBothResultsHaveError() {
Result resultLexical =
new Result(
new Query(),
ErrorMessage.createInternalServerError("Example lexical error"));
Result resultTensor =
new Result(
new Query(),
ErrorMessage.createInternalServerError("Example tensor error"));
RuntimeException exception =
assertThrows(
RuntimeException.class,
() -> {
hybridSearcher.raiseErrorIfPresent(resultLexical, resultTensor);
});
assertThat(exception.getMessage()).contains("Error in TENSOR search in RRF:");
assertThat(exception.getMessage()).contains("Example tensor error");
assertThat(exception.getMessage()).contains("\nError in LEXICAL search in RRF:");
assertThat(exception.getMessage()).contains("Example lexical error");
}

@Test
void shouldNotRaiseErrorIfNeitherResultHasError() {
Result resultLexical = new Result(new Query());
Result resultTensor = new Result(new Query());
hybridSearcher.raiseErrorIfPresent(resultLexical, resultTensor);
}
}
}
Loading