flutterfire icon indicating copy to clipboard operation
flutterfire copied to clipboard

refactor(vertexai): Split into separate libraries

Open natebosch opened this issue 1 year ago • 2 comments

The current approach uses part files which cannot be imported separately. One drawback of this approach is that any members which need to be referenced by test code must be public to the library. Using src/ libraries, as opposed to src/ part files, allows making APIs that are "package private" by convention - they are public to the library, but only exported through the src/ import.

One potential downside (alternatively seen as a benefit) of using separate libraries is that private members are truly private to their library, and we cannot make a member of a class private to the package. The convention for package imports only allows top level declarations to be considered non-public implementation details.

  • Remove imports from the top level library. Replace part lines with export of the same files.
  • Add explicit show clauses to the exports. This is what allows an API to be public when imported from the src/ library, but not exposed through the package public library.
  • Change some private _toGoogleAI<TypeName> methods to extension methods. Use the name toGoogleAI since the specific type names are clear from context.
  • Change some private _fromGoogleAI<TypeName> factory constructors to extension methods on the google AI SDk types. Name toVertex().
  • Remove some unused private methods that were filled in for consistency. More extension members can be added as needed.
  • Add an extension to expose the private field _googleAiModel from GenerativeModel.
  • Add a top level method to forward to the private GenerativeModel constructor.
  • Add TODO comments to stop exporting some methods that don't need to be public. These could have been private in the part pattern. For now they are exported for backwards compatibility, and we can remove them when we are ready for a major version bump.

natebosch avatar May 17 '24 00:05 natebosch

If you have any branches that don't merge cleanly with this I can hold off and land it whenever it's convenient.

This is a refactor I would recommend, but it's not urgent. Let me know if you have any questions about the reasoning or implications of this change.

If you want to go forward with this refactoring I can work on jumping through the CI hurdles.

natebosch avatar May 17 '24 00:05 natebosch

If you have any branches that don't merge cleanly with this I can hold off and land it whenever it's convenient.

This is a refactor I would recommend, but it's not urgent. Let me know if you have any questions about the reasoning or implications of this change.

If you want to go forward with this refactoring I can work on jumping through the CI hurdles.

This is great, thanks for doing this! I'd prefer you to go ahead and get this in first. It's much cleaner.

cynthiajoan avatar May 17 '24 02:05 cynthiajoan

I've added some API docs to these members for now. I'd recommend dropping the lint which requires this - it currently doesn't understand "package private" APIs (which these all are), and we have found it to be the case that some APIs don't have any useful information to add - and in those cases we prefer omitting the docs to adding redundant docs.

natebosch avatar May 21 '24 00:05 natebosch

[cloud_firestore/permission-denied] The caller does not have permission to execute the specified operation.

Are these flakes? It looks like there might also be some failures on master branch - is it OK to merge on red CI in this repo?

natebosch avatar May 21 '24 00:05 natebosch

@natebosch - you can ignore the firestore e2e as they relate to using second database which doesn't have emulator support and thus uses a live firebase project, I have app check enforcement on the firestore API as I am testing something out. When this PR gets to review state, I'll remove enforcement 😄, just ping me here (I think enforcement will be removed sooner to be honest).

russellwheatley avatar May 21 '24 08:05 russellwheatley

OK. I think this should be good to go.

natebosch avatar May 21 '24 16:05 natebosch