Skip to content

Revise FactCheckingEvaluator #4675

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Revise FactCheckingEvaluator #4675

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
13 changes: 10 additions & 3 deletions ...ient-chat/src/main/java/org/springframework/ai/chat/evaluation/FactCheckingEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
* @author Eddú Meléndez
* @author Mark Pollack
* @author guan xu
* @author Yanming Zhou
* @see Evaluator
* @see EvaluationRequest
* @see EvaluationResponse
Expand Down Expand Up @@ -97,7 +98,9 @@ public class FactCheckingEvaluator implements Evaluator {
* the default evaluation prompt suitable for general purpose LLMs.
* @param chatClientBuilder The builder for the ChatClient used to perform the
* evaluation
* @deprecated in favor of {@link #builder(ChatClient.Builder)}
*/
@Deprecated(forRemoval = true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be removed instead of deprecated?

Copy link
Member

Choose a reason for hiding this comment

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

We keep the deprecations latest for one release (even for milestone) before removing.

public FactCheckingEvaluator(ChatClient.Builder chatClientBuilder) {
this(chatClientBuilder, null);
}
Expand All @@ -108,7 +111,9 @@ public FactCheckingEvaluator(ChatClient.Builder chatClientBuilder) {
* @param chatClientBuilder The builder for the ChatClient used to perform the
* evaluation
* @param evaluationPrompt The prompt text to use for evaluation
* @deprecated in favor of {@link #builder(ChatClient.Builder)}
*/
@Deprecated
public FactCheckingEvaluator(ChatClient.Builder chatClientBuilder, @Nullable String evaluationPrompt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be private instead of deprecated?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this as is until we can completely remove it right after 1.1.0-M4.

Copy link
Member

Choose a reason for hiding this comment

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

I realise this is the constructor used by the builder for instantiation. We should mark this private then.

Assert.notNull(chatClientBuilder, "chatClientBuilder cannot be null");
this.chatClientBuilder = chatClientBuilder;
Expand All @@ -123,7 +128,9 @@ public FactCheckingEvaluator(ChatClient.Builder chatClientBuilder, @Nullable Str
* @return A FactCheckingEvaluator configured for Bespoke Minicheck
*/
public static FactCheckingEvaluator forBespokeMinicheck(ChatClient.Builder chatClientBuilder) {
return new FactCheckingEvaluator(chatClientBuilder, BESPOKE_EVALUATION_PROMPT_TEXT);
return FactCheckingEvaluator.builder(chatClientBuilder)
.evaluationPrompt(BESPOKE_EVALUATION_PROMPT_TEXT)
.build();
}

/**
Expand All @@ -149,8 +156,8 @@ public EvaluationResponse evaluate(EvaluationRequest evaluationRequest) {
return new EvaluationResponse(passing, "", Collections.emptyMap());
}

public static FactCheckingEvaluator.Builder builder() {
return new FactCheckingEvaluator.Builder();
public static FactCheckingEvaluator.Builder builder(ChatClient.Builder chatClientBuilder) {
return new FactCheckingEvaluator.Builder().chatClientBuilder(chatClientBuilder);
}

public static final class Builder {
Expand Down
9 changes: 5 additions & 4 deletions ...chat/src/test/java/org/springframework/ai/chat/evaluation/FactCheckingEvaluatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,28 @@
* Unit tests for {@link FactCheckingEvaluator}.
*
* @author guan xu
* @author Yanming Zhou
*/
class FactCheckingEvaluatorTests {

@SuppressWarnings("deprecation")
@Test
void whenChatClientBuilderIsNullThenThrow() {
assertThatThrownBy(() -> new FactCheckingEvaluator(null)).isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("chatClientBuilder cannot be null");

assertThatThrownBy(() -> FactCheckingEvaluator.builder().chatClientBuilder(null).build())
assertThatThrownBy(() -> FactCheckingEvaluator.builder(null).build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("chatClientBuilder cannot be null");
}

@SuppressWarnings("deprecation")
@Test
void whenEvaluationPromptIsNullThenUseDefaultEvaluationPromptText() {
FactCheckingEvaluator evaluator = new FactCheckingEvaluator(ChatClient.builder(mock(ChatModel.class)));
assertThat(evaluator).isNotNull();

evaluator = FactCheckingEvaluator.builder()
.chatClientBuilder(ChatClient.builder(mock(ChatModel.class)))
.build();
evaluator = FactCheckingEvaluator.builder(ChatClient.builder(mock(ChatModel.class))).build();
assertThat(evaluator).isNotNull();
}

Expand Down