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
Streamable HTTPCallable functions #14290
base: main
Are you sure you want to change the base?
Streamable HTTPCallable functions #14290
Changes from 25 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
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 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.
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.
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
emulatorURL
helper method for thestream
API'sat URL: URL
property. https://github.com/firebase/firebase-ios-sdk/blob/main/FirebaseFunctions/Tests/Integration/IntegrationTests.swiftThere 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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would have (genStream, genStreamError, genStreamNoReturn)
So I will add genStreamError and genStreamNoReturn cases as well.
SG??
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.
Check failure on line 407 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-15, Xcode_16.1, macOS)
Check failure on line 407 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-15, Xcode_16.1, macOS)
Check failure on line 407 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-15, Xcode_16.1, catalyst)
Check failure on line 407 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-15, Xcode_16.1, catalyst)
Check warning on line 430 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)
Check warning on line 430 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)
Check warning on line 438 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)
Check warning on line 438 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)
Check failure on line 445 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)
Check failure on line 445 in FirebaseFunctions/Tests/Unit/FunctionsTests.swift
GitHub Actions / spm-unit (macos-14, Xcode_15.4, iOS)