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
nested POJO extract #575
nested POJO extract #575
Conversation
8619efb
to
f15bfad
Compare
…7/langchain4j into poc/nested-json-object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
f15bfad
to
bd0486a
Compare
I didn't seen there was already a PR (#513) for nested objects. |
…7/langchain4j into poc/nested-json-object
…r final fields + add unit tests for ServiceOutputParser.outputFormatInstructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tenpigs267 great job, thank you!
if (name.equals("__$hits$__")) { | ||
if (name.equals("__$hits$__") | ||
|| java.lang.reflect.Modifier.isStatic(field.getModifiers()) | ||
|| java.lang.reflect.Modifier.isFinal(field.getModifiers())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this line is severely breaking.
Consider ServiceOutputParserTest.Person
: By simply making is class immutable (and adding a constructor so JSON deserialization works), outputFormatInstructions
will not return {\n}
.
Furthermore, the same problem occurs for Java Records.
I am not sure what the motivation for this line was, but I propose at the very least, it should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoand thanks for the heads up! Does it break Quarkus extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tenpigs267 BTW it also does not work if you have a recursion. So if I define it like this:
class Person {
private String firstName;
private String lastName;
private LocalDate birthDate;
private List<Person> parents;
It will go into endless recursion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoand thanks for the heads up! Does it break Quarkus extension?
It breaks some of our tests that use Java Records
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @langchain4j for #620 and sry for that.
For the recursive thing I'am going to add a PR with the code going through all involved classes and generating a list a class (and test it with openAI)
Following #575, here is a PR which add a set of visited classes sot that each class structure is defined only once (only when it is the root element class that is re-used, it is then defined a second time (see outputFormatInstructions_PersonWithParents unit test for a comprehensible example) Approch I suggested on #575 seemed less natural (navigate through classes first and then generate classes structure)
This PR enhance ServiceOutPutParser allowing outputFormatInstructions to document nested objects in jsonStructure.
Integration tests have been modified to add a nested address object. Maybe a dedicated test would be better?