-
Notifications
You must be signed in to change notification settings - Fork 16
fix: [PromptRegistry] Register new File converter in resttemplate
#660
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
base: main
Are you sure you want to change the base?
fix: [PromptRegistry] Register new File converter in resttemplate
#660
Conversation
Alternative Considered
I discarded the approach due to the side effect that a Spring class gets bound to our public API. Nonetheless, it's still a viable alternative to adding a |
| // export | ||
| final var exportedTemplate = controller.exportTemplate(); | ||
|
|
||
| final var resource = new ClassPathResource("prompt-template.yaml"); | ||
| final JsonNode expectedYaml = YAML_MAPPER.readTree(resource.getContentAsString(UTF_8)); | ||
| assertThat(YAML_MAPPER.readTree(exportedTemplate)).isEqualTo(expectedYaml); |
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.
What is this?
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.
The prompt-template.yaml is uploaded with importTemplate() method (part of same test) and then downloaded when exportTemplate() is invoked. Direct String comparison fails as yaml file's by nature don't have order to the properties defined in it. Consequently, returned file has a different order of properties and can't be directly compared without parsing. (Similar to json comparison)
I can also instead resort to checking existence of some string within the returned file content. I just went for a complete equivalence check.
| protected File readInternal( | ||
| @Nonnull final Class<? extends File> clazz, @Nonnull final HttpInputMessage inputMessage) | ||
| throws IOException, HttpMessageNotReadableException { | ||
| final var tempFile = File.createTempFile("tmp", ".bin"); |
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.
(Question)
Is there life-cycle in Temp File? Do we need to delete this file? Does it exist physically on disk - or virtually in memory?
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.
File.createTempFile() creates a physical file on disk in OS's temporary directory. My understanding was that if a File object is returned there needs to be a physical file it references.
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.
We cannot delete the File and its upto the caller of PromptTemplateApi.exportPromptTemplate() to do so.
Context
AI/ai-sdk-java-backlog#239.
Since RestTemplate is missing a built-in handler for
HttpMessagetoFileconverison, calls to export (download) endoint fails.Feature scope:
FileHttpMessageConverterDefinition of Done
Error handling created / updated & covered by the tests aboveAligned changes with the JavaScript SDKDocumentation updatedRelease notes updated