Repository navigation
feat(langchain4j): make modelName optional when ChatModel or StreamingChatModel is provided - #1126
Conversation
|
Hi @marttp, hank you for your contribution! We appreciate you taking the time to submit this pull request. I noticed some inconsistencies in the code formatting as per PR contribution policy. could you please address those? |
|
Hi @hemasekhar-p, thanks for the review. I've fixed the formatting (ran google-java-format via the Maven build) and squashed the branch. Please take another look when you have a chance 🙇♂️ |
|
@marttp, thank you for addressing those formatting and updating the PR. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
| if (!Objects.isNull(chatModel()) | ||
| && !Objects.isNull(chatModel().defaultRequestParameters())) { | ||
| modelName(chatModel().defaultRequestParameters().modelName()); | ||
| } else if (!Objects.isNull(streamingChatModel()) | ||
| && !Objects.isNull(streamingChatModel().defaultRequestParameters())) { | ||
| modelName(streamingChatModel().defaultRequestParameters().modelName()); | ||
| } |
There was a problem hiding this comment.
MEDIUM: the streaming model's name is never tried when the chat model has none (LangChain4j.java#L142-L148)
The else if branch only checks that the chat model's parameters object is non-null, not that it carries a model name. Because ChatModel.defaultRequestParameters() returns DefaultChatRequestParameters.EMPTY by default, this throws even when the streaming model declares a name:
LangChain4j.builder()
.chatModel(new ChatModel() {}) // default parameters, no model name
.streamingChatModel(streamingModelWithName)
.build(); // IllegalStateExceptionResolving the name per model fixes this:
public LangChain4j build() {
if (modelName() == null) {
String name = chatModel() == null ? null : nameOf(chatModel().defaultRequestParameters());
if (name == null && streamingChatModel() != null) {
name = nameOf(streamingChatModel().defaultRequestParameters());
}
if (name == null) {
throw new IllegalStateException(
"Set modelName(), or use a ChatModel or StreamingChatModel whose"
+ " defaultRequestParameters() declare a model name");
}
modelName(name);
}
return autoBuild();
}
private static @Nullable String nameOf(@Nullable ChatRequestParameters parameters) {
return parameters == null ? null : parameters.modelName();
}| if (modelName() == null) { | ||
| throw new IllegalStateException( | ||
| "modelName is required. Either set it explicitly via modelName() or provide a ChatModel/StreamingChatModel"); |
There was a problem hiding this comment.
MEDIUM: the failure path is untested, and its message is misleading (LangChain4j.java#L151-L153)
The three new tests only cover cases where a name is found. Please also add tests for when no source provides a name, and for a chat model without a name paired with a streaming model that has one (the case above). The current exception message asks for a ChatModel/StreamingChatModel, even though providing one without a model name hits the same failure; the message in the snippet above states what is actually missing.
|
Please also update commit message to follow conventional commits - likely "feat:" here |
…gChatModel is provided
|
@damianmomotgoogle thank you. I made an update 🙇♂️ |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
N/A
Testing Plan
Unit Tests:
Command
mvn test -pl contrib/langchain4j -amManual End-to-End (E2E) Tests:
I created a sample project to verify session interaction
And triggered by this
E2E Result
Checklist
Additional context
The LangChain4j builder now auto-detects modelName from the underlying ChatModel or StreamingChatModel via
defaultRequestParameters().modelName(), making it optional when the model already exposes it. Explicit modelName() still takes precedence for backward compatibility. If model name cannot be resolved from any source, the builder fails fast with a clear IllegalStateException.