-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Streamable HTTPCallable functions #14290
base: main
Are you sure you want to change the base?
Changes from all commits
231d602
a14d964
a92d7c2
10bec1d
53a2aab
758fbed
93b6c8b
a7e8fe8
6d59fcd
51f02b8
7b61076
a95449e
cdc49ee
426b6bc
9cb0a5e
ad31052
6ee9000
1ffe73d
9fcd91e
177aa8e
f4d678b
74557e7
18f748b
4f956fb
4edc0ad
e50f69c
7356cf9
aed47d6
f6c6cff
75a7574
adf7366
9ef7411
4ee820e
fd68f01
f27bf07
1ffa4f0
80f0991
756dc26
f031c1f
0df7f8d
231c7dd
be80d63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With #14357 merged, we should be able to get these passing in CI now. This can be done by moving these new tests to the integration test file and use the |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -358,4 +358,73 @@ | |
} | ||
waitForExpectations(timeout: 1.5) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think another good test case would be to test when an error is thrown. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we would have (genStream, genStreamError, genStreamNoReturn) SG?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. |
||
func testGenerateStreamContent() async throws { | ||
let options = HTTPSCallableOptions(requireLimitedUseAppCheckTokens: true) | ||
|
||
let input: [String: Any] = ["data": "Why is the sky blue"] | ||
let stream = try await functions!.stream( | ||
at: URL(string: "http://127.0.0.1:5001/demo-project/us-central1/genStream")!, | ||
withObject: input, | ||
options: options, | ||
timeout: 4.0 | ||
) | ||
let result = try await response(from: stream) | ||
XCTAssertEqual( | ||
result, | ||
[ | ||
"chunk hello", | ||
"chunk world", | ||
"chunk this", | ||
"chunk is", | ||
"chunk cool", | ||
"hello world this is cool", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect this final concatenation was a result of GTMSessionFetcher handling the streamed results. I don't expect it with URLSession and AsyncBytes |
||
] | ||
) | ||
} | ||
|
||
func testGenerateStreamContentCanceled() async { | ||
let options = HTTPSCallableOptions(requireLimitedUseAppCheckTokens: true) | ||
let input: [String: Any] = ["data": "Why is the sky blue"] | ||
|
||
let task = Task.detached { [self] in | ||
let stream = try await functions!.stream( | ||
at: URL(string: "http://127.0.0.1:5001/demo-project/us-central1/genStream")!, | ||
withObject: input, | ||
options: options, | ||
timeout: 4.0 | ||
) | ||
|
||
let result = try await response(from: stream) | ||
// Since we cancel the call we are expecting an empty array. | ||
XCTAssertEqual( | ||
result, | ||
[] | ||
) | ||
} | ||
// We cancel the task and we expect a null response even if the stream was initiated. | ||
task.cancel() | ||
let respone = await task.result | ||
XCTAssertNotNil(respone) | ||
} | ||
} | ||
|
||
private func response(from stream: AsyncThrowingStream<HTTPSCallableResult, | ||
any Error>) async throws -> [String] { | ||
var response = [String]() | ||
for try await result in stream { | ||
Check failure on line 415 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift GitHub Actions / spm-unit (macos-15, Xcode_16.1, tvOS)
Check failure on line 415 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift GitHub Actions / spm-unit (macos-15, Xcode_16.1, macOS)
Check failure on line 415 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift GitHub Actions / spm-unit (macos-15, Xcode_16.1, macOS)
Check failure on line 415 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift GitHub Actions / spm-unit (macos-15, Xcode_16.1, catalyst)
Check failure on line 415 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift GitHub Actions / spm-unit (macos-15, Xcode_16.1, catalyst)
|
||
// First chunk of the stream comes as NSDictionary | ||
if let dataChunk = result.data as? NSDictionary { | ||
for (key, value) in dataChunk { | ||
response.append("\(key) \(value)") | ||
} | ||
} else { | ||
// Last chunk is the concatenated result so we have to parse it as String else will | ||
// fail. | ||
if let dataString = result.data as? String { | ||
response.append(dataString) | ||
} | ||
} | ||
} | ||
return response | ||
} |
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 just came across one subtle issue is that GTMSessionFetcher is returning the entire data result at once rather than chunks of the stream. It doesn't look like GTMSessionFetcher supports receiving streams so we may have to change this up and use the standard library's URLSession and URLSession.AsyncBytes
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.
Sure, I'll get to it. I was trying to keep it as closest as possible to HTTPCallable. It is subtle but quite important.
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'd recommend following what the Vertex SDK does. Here is the method that loads the request stream: https://github.com/firebase/firebase-ios-sdk/blob/main/FirebaseVertexAI/Sources/GenerativeAIService.swift#L82-L176
I'm thinking the implementation here won't be as complex.
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.
SGTM.