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

refactor(vertexai): Split into separate libraries #12794

Merged
merged 11 commits into from
May 22, 2024

Conversation

natebosch
Copy link
Contributor

@natebosch natebosch commented May 17, 2024

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.

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 `_gooleAiModel` 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
Copy link
Contributor Author

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.

@cynthiajoan cynthiajoan changed the title Split into separate libraries refactor(vertexai): Split into separate libraries May 17, 2024
@cynthiajoan
Copy link
Collaborator

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.

@natebosch
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@russellwheatley
Copy link
Member

@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).

@natebosch natebosch marked this pull request as ready for review May 21, 2024 16:08
@natebosch
Copy link
Contributor Author

OK. I think this should be good to go.

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.

Small nit for spelling, but LGTM

…i.dart

Co-authored-by: Russell Wheatley <russellwheatley85@gmail.com>
@cynthiajoan cynthiajoan merged commit 85a517f into firebase:master May 22, 2024
19 of 20 checks passed
@natebosch natebosch deleted the libraries-over-parts branch May 22, 2024 23:38
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.

None yet

3 participants