Skip to content

feat: create new Vertex AI for Firebase plugin. #12728

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

Merged
merged 44 commits into from
May 10, 2024
Merged

Conversation

cynthiajoan
Copy link
Collaborator

@cynthiajoan cynthiajoan commented May 1, 2024

Description

Add firebase_vertexai plugin. This plugin will provide flutter developer to have access to Vertex AI service. More details in README.md of the package.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@cynthiajoan cynthiajoan changed the title Draft PR to create new firebase vertex ai plugin. feat: create new Vertex AI for Firebase plugin. May 7, 2024
@cynthiajoan cynthiajoan marked this pull request as ready for review May 8, 2024 04:52
Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @cynthiajoan

A few things to note:

I am aware that the timeline is quite short so I've made some suggestions. I think that this being a dev release gives us more flexibility with future changes, and it might be worth pointing out that the API could change in the future.

For instance, I'm not sure we should have a dependency on App Check as it forces the user to have it + app check plugin initialisation, particularly as we're only interested in the token. There are ways around this issue that would require some internal changes and we can think about that before making it general release.

There are a number of private methods that don't appear to be used but I'm guessing they're for future releases. In any event, they can also be removed in a future release.

I don't think the plugin needs to extend the FirebasePluginPlatform but this will cause issue with the mock tests so it can be left and removed at a later time if we wish.

@cynthiajoan
Copy link
Collaborator Author

Thank you for the PR, @cynthiajoan

A few things to note:

I am aware that the timeline is quite short so I've made some suggestions. I think that this being a dev release gives us more flexibility with future changes, and it might be worth pointing out that the API could change in the future.

For instance, I'm not sure we should have a dependency on App Check as it forces the user to have it + app check plugin initialisation, particularly as we're only interested in the token. There are ways around this issue that would require some internal changes and we can think about that before making it general release.

There are a number of private methods that don't appear to be used but I'm guessing they're for future releases. In any event, they can also be removed in a future release.

I don't think the plugin needs to extend the FirebasePluginPlatform but this will cause issue with the mock tests so it can be left and removed at a later time if we wish.

Thanks Russell. I agree with the app check part, I will point out the sdk is in preview status and apis might be changing.

@Lyokone
Copy link
Contributor

Lyokone commented May 10, 2024

More on the app_check part, we can probably remove the changes made to the example App Check app, so we don't get a weird changelog for app check on the next release.
Otherwise I agree with Russell's review 👍

@cynthiajoan
Copy link
Collaborator Author

More on the app_check part, we can probably remove the changes made to the example App Check app, so we don't get a weird changelog for app check on the next release. Otherwise I agree with Russell's review 👍

removed! Thanks for the review

@cynthiajoan cynthiajoan requested a review from Lyokone May 10, 2024 15:32
Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

I think tests are failing because of the name

Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

CI failures are unrelated 👍

@russellwheatley russellwheatley merged commit abb10ff into master May 10, 2024
20 of 23 checks passed
@russellwheatley russellwheatley deleted the vertex_ai branch May 10, 2024 18:15
@@ -0,0 +1,16 @@
# firebase_vertexai_example

A new Flutter project.
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Update the file with specifics, or consider removing it if it's not useful.

// See the License for the specific language governing permissions and
// limitations under the License.

library firebase_vertexai;
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] If we don't have a library level doc comment or annotation, we can omit the library line.
We can use URIs for the part of lines.

At some point it would be worth considering a refactor to stop using part and part of, but that can happen out of band.

show FirebasePluginPlatform;
import 'package:google_generative_ai/google_generative_ai.dart' as google_ai;
// ignore: implementation_imports, tightly coupled packages
import 'package:google_generative_ai/src/model.dart' as google_ai_model;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider updating this import to src/vertex_hooks.dart. We export the model creating method there now so you can import all of the vertex customizations from one place.

// See the License for the specific language governing permissions and
// limitations under the License.

part of firebase_vertexai;
Copy link
Contributor

Choose a reason for hiding this comment

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

part of '../firebase_vertexai.dart';

const _defaultLocation = 'us-central1';

/// Default timeout duration, 30 minutes in millisecond
const int defaultTimeout = 1800000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentionally public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not. However it's referenced in the test file, and I'm still learning how to access private attributes in test folder

@firebase firebase locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants