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
SpeziLLMOpenAI: Replace MacPaw/OpenAI With Generated API Calls #64
base: main
Are you sure you want to change the base?
SpeziLLMOpenAI: Replace MacPaw/OpenAI With Generated API Calls #64
Changes from 1 commit
0cfbf29
d340a10
ce640b4
6ac80c2
0b02090
088b52d
40acb59
cc31e35
a6035ef
d9411be
52baf18
98c84fc
c6534a5
5b47220
e64fb9b
50db906
1a6cc12
6dd0f95
630d1dc
9cba800
02f796e
e14f3d3
5021c84
e82370d
6741f14
823bf78
0ffcfd6
0c437cc
6c7a89a
b1b95b5
95c2979
0a789e6
0ad1776
48c3aab
6471784
91b3f8d
e39d202
3a994ee
7fea2b4
fd524b0
80e133f
5a19c35
13b7bd2
16644d0
3ded7a0
2a5269b
271b255
55897f8
10fcb63
37b2b60
42f9079
11d0d12
ec00d56
b357d2d
064d56c
118505b
fb02c7f
edc7582
dcad43f
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.
I am wondering if we can add a nicely typed type for this instead of a dictionary; it can always map to a dictionary under the hood. Would be cool to avoid loosing that type-safe element?
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.
Previously, SpeziLLMOpenAI wrapped around the Swift types provided by the
OpenAI
package, which would then eventually be passed to the API.With the OpenAI OpenAPI spec, such types aren't generated, but the JSON schemas are instead validated for correctness as they're being encoded in the OpenAPIObjectContainer type.
Introducing such wrapper types again would require precise alignment with the OpenAI, which would make it, I could imagine, harder to maintain over time.
I could imagine that’s one reason why the official OpenAI Python package, which is also generated from the OpenAI OpenAPI specification, does not offer wrapper types either, AFAICT.
What do you think?
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 think adding an extension initializer/function that takes the well-typed arguments of one wants to use them would be beneficial and would avoid issues with string keys that are not correct or malformatted. Still allowing to pass in a dictionary might be an escape hatch that we can still provide. The OpenAPI surface is quite stable and if we use e.g. an enum for the type of the parameter can also have an
other
case with an associated string value.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 most definitely agree with @PSchmiedmayer here, I would follow the typical Swift paradigm and provide as much type safety as possible.
As mentioned by @PSchmiedmayer, I would implement well-typed inits / functions etc. that then map to the underlying
String
dictionary. And yes, an escape hatch that passes the raw dictionary might be beneficial!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.
Just to be clear and because I'm a little clueless about how to implement this easily: the only way I'm seeing here is to re-implement these two types from the MacPaw OpenAI package that SpeziLLM was using previously, and then reverting the changes around the initialisers accordingly.
SpeziLLM/Sources/SpeziLLMOpenAI/FunctionCalling/LLMFunctionParameterWrapper.swift
Lines 12 to 15 in 26b1e07
I was not able to find a definitive documentation for the fileds that the OpenAI API accepts here, including the ones that are currently support by SpeziLLM, e.g.,
minItems
,maxItems
,uniqueItems
.The function calling documentation mentions none of them: https://platform.openai.com/docs/guides/function-calling
What do you think?
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.
Probably a more descriptive error message would be beneficial here.
In general, I think we shouldn't just ignore this error! (how does the current procedure play out down the road when OpenAI function calling parameters are decoded and injected into the Swift DSL?)
The downside is a potentially throwing init, which we definitely don't want here.
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.
Once we've worked out #64 (comment), we can probably apply the same solution here, I hope.
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.
Same feedback applies everywhere