Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: [Orchestration] Configuration as JSON String #177
feat: [Orchestration] Configuration as JSON String #177
Changes from all commits
beffd97
190d8d5
32788ab
ef3a862
cc86c74
2165e90
cf6494a
c280ae3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Your suggested code change results in the following three methods on client:
(Question)
Is it not possible to derive reuse existing method and allow for
OrchestrationModuleConfig
to be created fromString configJson
? That would look a little cleaner, maybe(?)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.
I also thought about this approach. But unfortunately not, because the JSON string is expected to contain things not yet supported by
OrchestrationModuleConfig
or it's individual config objects, i.e. throughout the entire JSON. That includes cases ofoneOf
, e.g. TemplateRef.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.
👍
Let's keep this as beta before the alignment with e.g., Launchpad.
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.
An OrchestrationPrompt is not the right object then. Why not accept a map of input params?
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.
Because that is missing the (optional) messages history.
I'd argue it is. We will have further cases where passing messages is not allowed, e.g. when using TemplateRef. If we want we could create another Prompt object for that to make this distinction, but so far I figured it's not worth it.
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.
(Major/Question)
Not sure, but wouldn't it make more sense applying the module config JSON to client instead of per request?
Seems like the only place where
orchestration_config
is provided, it could also be an optional parameter that is passed along the convenience API(?)(My hidden motivation is to challenge this method a little.)
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.
Hm not exactly sure what you mean.. Do you mean setting the config on the
OrchestrationClient
object instead? Or something else?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.
Yes.
Boiling this down to the question: Is
orchestration_config
to be expected request specific or client specific?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.
Essentially same as the
OrchestrationConfig
object: It depends how much, if any, config changes per request. But the tendency is it is tied to the client, i.e. for typical use cases the majority of config will stay the same across multiple requests.However, we decided to not allow config per client to make the API simpler. So the same strategy is applied for this method too. If we want to, we can still change this in the future though.
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.
Understood.
To me, from a user perspective, it would feel weird to pass a
String
object along (over multiple controller classes potentially) just to pass to my request invocations.But it's difficult to argue about client / request specific data handling, when I haven't yet seen a proper use-case "in the wild". Maybe we will discuss this again, after having users first interact with that.
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.
I would make the prompt as optional parameter.
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.
It's not optional, it has to be given. In this unit test I'm just skipping the effort to give params in the map
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.
You are right, so we can discuss the API signature/design 😄
Please have a look at the following json, that I copied from the Launchpad.
I would assume, I've got everything from the JSON and should be able to run it with a function like
The second param (
prompt
in this case) is only needed (so it should be optional), when I want to pass ainputParam
or provide message history like the following screenshot: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.
Ah true, Launchpad doesn't force you to use any variables. Okay, will update 👍🏻
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.
After thinking about this again, I actually think the use case for this is not very strong. Because if there are no input params or messages history, prompt execution will always yield the same or very similar results (depending on the temperature setting).
The same is true when using template references. If we really want to support such usage, we could consider allowing
new OrchestrationPrompt()
for these cases. But making theprompt
itself optional would mean we add 2-3 method overloads for non-productive cases..