Skip to content

Simplify the implementation of ChatClient-based interfaces #1663

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.

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

Conversation

jxblum
Copy link
Contributor

@jxblum jxblum commented Nov 4, 2024

Provide sensible defaults for overloaded methods in PromptUserSpec, PromptSystemSpec, AdvisorSpec, CallResponseSpec, ChatClientRequestSpec, and Builder interfaces.

Provide sensible defaults for overloaded methods in PromptUserSpec, PromptSystemSpec, AdvisorSpec, CallResponseSpec, ChatClientRequestSpec, and Builder interfaces.

Closes spring-projects#1663
@markpollack
Copy link
Member

markpollack commented Nov 6, 2024

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.

@jxblum
Copy link
Contributor Author

jxblum commented Nov 13, 2024

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 the PromptUserSpec interface (and similar) there is the text(:Resource, :Charset) (here) and params(: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.

@jxblum
Copy link
Contributor Author

jxblum commented Nov 13, 2024

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.

@jxblum
Copy link
Contributor Author

jxblum commented Nov 14, 2024

@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.

@jxblum
Copy link
Contributor Author

jxblum commented Nov 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants