Provide sensible defaults for overloaded methods in PromptUserSpec
, PromptSystemSpec
, AdvisorSpec
, CallResponseSpec
, ChatClientRequestSpec
, and Builder
interfaces.
Comment From: markpollack
I'd like to keep the interfaces without any defaults as in WebClient and other spring projects. This PR also introduces a conflict in that the single implementation of the existing spec classes have different behavior it seems at first glance. I took a look at PromptUserSpec
for example.
@Override
public PromptUserSpec text(String text) {
Assert.hasText(text, "text cannot be null or empty");
this.text = text;
return this;
}
vs.
default PromptUserSpec text(String text) {
Charset defaultCharset = Charset.defaultCharset();
return text(new ByteArrayResource(text.getBytes(defaultCharset)), defaultCharset);
}
What is wrong with using the string as supplied i the current impl?
In anycase, these impls are used in one place, so if there are changes to make in the impl, i'd do it there and not default method interfaces.
Comment From: jxblum
Thank you for the feedback @markpollack.
Keeping the interfaces inside ChatClient
(and other areas of Spring AI) void of implementation (i.e. without defaults) is reasonable. I actually considered this before submitting the PR and wondered whether I might add abstract base classes instead. Java's own Collection framework is a really good example of providing many abstract base classes supporting implementations.
I tend to provide defaults when:
1) The default implementation is very simple to implement (only a few lines)
2) The functionality can be easily and reliably implemented in terms of another method
3) Reduces effort and promotes consistency across implementations
4) Enables the use of functional interfaces (aka @FunctionalInterface
) as Lambda expressions
NOTER: #4 is not as applicable in this case since the
ChatClient
(sub) interfaces provides several related but distinct methods. For example, in thePromptUserSpec
interface (and similar) there is thetext(:Resource, :Charset)
(here) andparams(:Map)
methods... related but distinct ops with a couple of "overloaded" variants that do not differ much in implementation and therefore can follow the most complete method signature.
Regarding...
What is wrong with using the string as supplied i the current impl?
Nothing, other than using the given String
directly is an implementation detail.
Any implementation is free to still override a default implementation, necessarily based on internal data structure, performance, or all sorts of reasons. But the key is, implementations do not need to override and explicitly implement every overloaded variant, keeping the implementations focused on the singular function rather than form.
A lot of the methods in the ChatClient
interfaces are for "convenience". They are not functionality independent.
Finally, Spring is an excellent example of the Open/Closed principle, and making the task of implementation by downstream frameworks, libraries or tooling in addition to an application's occasional need for extension simpler is generally a good thing.
Food for thought.
Comment From: jxblum
FYI, in the process of writing my book, I have thought of many extensions and areas for improvement in Spring AI, while considering all angles: libraries, frameworks, applications and tooling implementations.
Comment From: jxblum
@markpollack
I should probably add that, in several cases, Spring AI does use default methods on interfaces.
For example, ChatModel
and StreamingChatModel
both use default methods, as does ChatMemory
.
In fact, this is quite common throughout Spring AI's codebase.
StructuredOutputConverter
DocumentReader
&DocumentWriter
Evaluator
VectorStore
here and here.- And on and on...
Comment From: jxblum
1 last comment...
Declaring default methods in interfaces does not change the default implementations/behavior already provided by Spring AI in the DefaultXyz classes, for example, DefaultChatClient
.
In particular, the text(:String):PromptUserSpec
method (source) that you referred to above will still be used over the default method provided in the interface. And, the (acquired) ChatClientBuilder
used to build and construct a ChatClient
(also used in auto-configuration provided by Spring AI) will still build a DefaultChatClient
, for which all the "default", associated ChatClient
sub-interface implementations are maintained.
So, this PR changes nothing, and only provides convenience.