Skip to content
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

Draft
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

eBlender
Copy link

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

  • Read the contribution guidelines (CONTRIBUTING.md).
  • If this has been discussed in an issue, make sure to link to the issue here.
    If not, go file an issue about this before creating a pull request to discuss.

Testing

  • Make sure all existing tests in the repository pass after your change.
  • If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in a feature request so that we
    can discuss it together.

Add initial support for Streamable functions.
Change call to AsyncThrowingStream<HTTPSCallableResult, Error>
testing check.sh
Remove old test function.
Remove old test
Add a full working test for stremableFunction. Refactor.
Copy link

google-cla bot commented Dec 27, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Add Json capabilities to parse an HTTP result back and forth.

Updating Unit tests.
Add doc for processResponseDataForStreamableContent.
Update func to have a callback.
Update UnitTests
Remove spaces.
@eBlender eBlender changed the title I os stremable functions iOS streamable HTTPCallabe functions Jan 3, 2025
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. We'll need an approved API review before doing a detailed code review. However, in the meantime, it would be helpful to make this PR a clean diff to main that can run CI.

@eBlender
Copy link
Author

eBlender commented Jan 6, 2025

@paulb777 Thanks Paul, Done.

Fetch Main branch.
Project clean up.
Updated to renamed code. From callableResultFromResponse to callableResult
Run style.sh
Fix concurrency " mutation of captured var 'response' in concurrently-executing code" and typos.
Accept suggestion. Thanks.

Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
@ncooke3
Copy link
Member

ncooke3 commented Jan 16, 2025

For the check CI failure, use the scripts/style.sh script.

You can use it a few ways, but I like to either pass it a list of files:

scripts/style.sh FirebaseFunctions/Tests/Unit/FunctionsTests.swift

Or periodically style all of the files I touched in my branch:

git diff --name-only main | xargs scripts/style.sh

Then git add and commit the result.

If the script fails, it's likely due to missing packages, you'll need:

brew install clang-format@19
brew install mint

@eBlender
Copy link
Author

eBlender commented Jan 16, 2025

@paulb777 and @ncooke3 Thanks for your comments and help.

@eBlender eBlender changed the title iOS streamable HTTPCallabe functions Streamable HTTPCallabe functions Jan 16, 2025
Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make result a constant since it doesn't need to change past initialization

FirebaseFunctions/Tests/Unit/FunctionsTests.swift Outdated Show resolved Hide resolved
FirebaseFunctions/Tests/Unit/FunctionsTests.swift Outdated Show resolved Hide resolved
FirebaseFunctions/Tests/Unit/FunctionsTests.swift Outdated Show resolved Hide resolved
FirebaseFunctions/Tests/Unit/FunctionsTests.swift Outdated Show resolved Hide resolved
eBlender and others added 4 commits January 16, 2025 14:45
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Accepted suggestion.

Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Comment on lines +490 to +491
let rawData = try await fetcher.beginFetch()
return try callableResultFromResponseAsync(data: rawData, error: nil)
Copy link
Member

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

Copy link
Author

@eBlender eBlender Jan 16, 2025

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.

Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

@@ -358,4 +358,73 @@ class FunctionsTests: XCTestCase {
}
waitForExpectations(timeout: 1.5)
}

Copy link
Member

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.

Copy link
Author

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??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

Mark it as public.

Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
"chunk this",
"chunk is",
"chunk cool",
"hello world this is cool",
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

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 the stream API's at URL: URL property. https://github.com/firebase/firebase-ios-sdk/blob/main/FirebaseFunctions/Tests/Integration/IntegrationTests.swift

@paulb777 paulb777 changed the title Streamable HTTPCallabe functions Streamable HTTPCallable functions Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants