Port PromptTemplate #6
Conversation
| fileSystem.source(templateFile).use { source -> | ||
| source.buffer().use { buffer -> | ||
| val template = buffer.readUtf8() | ||
| val config = Config.orThrow(template, inputVariables) |
There was a problem hiding this comment.
All the smart-constructors for PromptTemplate are turning NonEmptyList<InvalidTemplate> into an exception, we can keep this as Raise<NonEmptyList<InvalidTemplate>>.
Perhaps better is Raise<InvalidTemplate>, where the Config smart-constructors is already formatting the final message from NonEmptyList<String>.
Then we can turn the signature into something like Raise<PromptTemplate.IncorrectConfig>, and create an error hierarchy if we need to with more errors.
WDYT?
There was a problem hiding this comment.
I removed all the exceptions, and orThrow and pushed them to the smart-constructors. You can always wrap in either { }.getOrNull() or recover({ }) { null } or we can expose a Either<InvalidTemplate, A>.getOrThrow extension within the module so dealing with the error at least becomes explicit.
| override suspend fun format(variables: Map<String, String>): String { | ||
| val mergedArgs = mergePartialAndUserVariables(variables, config.inputVariables) | ||
| return when (config.templateFormat) { | ||
| TemplateFormat.Jinja2 -> TODO() |
There was a problem hiding this comment.
This is currently not implemented in lanchain4s and is resulting in NoSuchElementException, so I am keeping it here as TODO for now.
There was a problem hiding this comment.
I removed, this from the enum and raised a PR for Scala to do the same. I think it's better to not have this until we actually implement it.
| suspend fun fromFile( | ||
| templateFile: Path, | ||
| inputVariables: List<String>, | ||
| fileSystem: FileSystem |
There was a problem hiding this comment.
This only exists for JVM, Native & NodeJS.
If we drop the browser target then we can provide a default argument of FileSystem.SYSTEM in this function, the Okio library also provides the test instance as well. We can also keep it as is, since browser cannot provide an instance for this it cannot call the method.
There was a problem hiding this comment.
At the moment, we don't need to load templates from files. I think it's better to keep the browser support I suspect in the browser there is some kind of API to access local storage if you have been granted permission which we could use in place of speaking in terms of File
There was a problem hiding this comment.
@raulraja it currently still supports browser, but from a browser sourcceSet you cannot provide a FileSystem instance. square/okio#1070.
I'll provide overloads of this function from JVM, Native and NodeJS that remove that argument.
There was a problem hiding this comment.
Could only do this for native, and JVM for now. Cannot define a separate NodeJS sourceSet, since there is no commonizer between NodeJS and Browser. https://youtrack.jetbrains.com/issue/KT-47038
Super tiny issue though, still quite easy to call it from node by passing FileSystem.SYSTEM.
|
@gutiory @franciscodr @realdavidvega PR is ready for review. |
| suspend fun fromFile( | ||
| templateFile: Path, | ||
| inputVariables: List<String>, | ||
| fileSystem: FileSystem |
There was a problem hiding this comment.
At the moment, we don't need to load templates from files. I think it's better to keep the browser support I suspect in the browser there is some kind of API to access local storage if you have been granted permission which we could use in place of speaking in terms of File
| { validate(template, placeholders.toSet() - variables.toSet(), "missing") }, | ||
| { validateDuplicated(template, placeholders) } | ||
| ) { _, _, _ -> Config(variables, template) } | ||
| }.mapLeft { InvalidTemplate(it.joinToString(transform = InvalidTemplate::reason)) } |
There was a problem hiding this comment.
I didn't know the transform argument to join a List of elements into a single String. It looks neat 😄
This PR ports the
PromptTemplatefrom langchain4s.