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

Project proposal for Firebase ML Publisher custom component #69

Merged
merged 4 commits into from
Sep 16, 2021
Merged

Project proposal for Firebase ML Publisher custom component #69

merged 4 commits into from
Sep 16, 2021

Conversation

deep-diver
Copy link
Contributor

Proposal for project idea discussed in #59

@deep-diver
Copy link
Contributor Author

@rcrowe-google
Should I attend the regular meeting in person?

@rcrowe-google
Copy link
Collaborator

@deep-diver If you can, that would be great.


The implementation details
- Define a custom Python function-based TFX component. It takes the following parameters from a previous component.
- The URI of the pushed model from TFX Pusher component.
Copy link
Member

Choose a reason for hiding this comment

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

Why Pushed model? Why not a Trained Model directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an option I guess?

My understanding is that TFX Trainer just saved the trained model, and Pusher manages the different versions(correct me if I am wrong).

Since there are more after Trainer's job is done. For example, we don't know if the trained model is good enough, so we add Evaluator component with Resolver node. When the model passes thresholds defined in eval_config, the model is ready to be pushed.

Copy link

Choose a reason for hiding this comment

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

AFAIK, trainer could generate tflite models, and evaluator could handle tflite models.
https://www.tensorflow.org/tfx/tutorials/tfx/tfx_for_mobile

From pusher perspective, Pushed model type artifacts are for archiving already pushed models.
I think this new custom component should follow the standard pusher's interface instead of reading pusher's output, since this is another custom pusher.

https://github.com/tensorflow/tfx/blob/921167ce1b53e7f0632bffacd20304cfed82d901/tfx/types/standard_component_specs.py#L284-L296

We don't have infra validator which supports tflite models for now, but we could have this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for leaving the comment, and I think you are right.

@nikelite

Suggested change
- The URI of the pushed model from TFX Pusher component.
- It should follow the standard Pusher's interface.

@casassg please let me know your thought

Copy link
Member

Choose a reason for hiding this comment

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

I agree w the conclusion reached. Let's make sure to modify the proposal w the new interface.

The implementation details
- Define a custom Python function-based TFX component. It takes the following parameters from a previous component.
- The URI of the pushed model from TFX Pusher component.
- Requirements from Firebase ML (credential JSON file path, Firebase temporary-use GCS bucket). Please find more information from [Before you begin section](https://firebase.google.com/docs/ml/manage-hosted-models#before_you_begin) in the official Firebase document.
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid overall setting credential JSON file path in the component. User can login locally via gcloud auth application-default login or setting GOOGLE_APPLICATION_CREDENTIALS as mentioned in https://firebase.google.com/docs/admin/setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how I can do that.
Since every component runs on a docker container, the environment variable should be set in Dockerfile then? (please let me know how to setup ENV variable for TFX component).

but still, we can make this parameter Optional. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

When using Kubernetes on GKE, you can use Workload identity to set up your pods to automatically auth as the user (see: https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity)

Otherwise, you can set it up using https://github.com/tensorflow/tfx/blob/master/tfx/orchestration/kubeflow/kubeflow_dag_runner.py#L78 (which is preferred over setting a GCS path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@casassg

I am currently working on implementing this component, but I can't get it working correctly in Vertex AI Pipeline environment.

Just like your comments, I thought Default Application Credential should work OK, but I get the error message like below by complaining to include private key as the credential.

I gave appropriate permissions to the Service Account which is compute such as Firebase Admin (it is not permission error anyways). I have tried out the same thing step-by-step in Vertex AI Workbench environment as well, and I get the same error.

Do you have any clue?

AttributeError: you need a private key to sign credentials.the credentials you are currently using <class 'google.auth.compute_engine.credentials.Credentials'> just contains a token. see https://googleapis.dev/python/google-api-core/latest/auth.html#setting-up-a-service-account for more details.

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 we can figure this out as we merge the implementation as it's hard to debug without the code

With Firebase ML, we can guarantee that mobile devices can be equipped with the latest ML model without explicitly embedding binary in the project compiling stage. We can even A/B test different versions of a model with Google Analytics when the model is published on Firebase ML.

## Project Implementation
Firebase ML Publisher component will be implemented as Python function-based component. You can find the [actual source code](https://github.com/sayakpaul/Dual-Deployments-on-Vertex-AI/blob/main/custom_components/firebase_publisher.py) in my personal project.
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider using tfx.io_utils instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I will take a look at it when polishing the code.

Comment on lines 38 to 41
## Project Dependencies
The implementation will use the following libraries.
- [Firebase Admin Python SDK](https://github.com/firebase/firebase-admin-python)
- [Python Client for Google Cloud Storage](https://github.com/googleapis/python-storage)
Copy link
Member

Choose a reason for hiding this comment

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

Can we get the versions of the PyPi deps here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The versions are 5.0.2 and 1.42.0 respectively.

Suggested change
## Project Dependencies
The implementation will use the following libraries.
- [Firebase Admin Python SDK](https://github.com/firebase/firebase-admin-python)
- [Python Client for Google Cloud Storage](https://github.com/googleapis/python-storage)
## Project Dependencies
The implementation will use the following libraries.
- [Firebase Admin Python SDK](https://github.com/firebase/firebase-admin-python) >= 5.0.2
- [Python Client for Google Cloud Storage](https://github.com/googleapis/python-storage) >= 1.42.0

Copy link
Member

@casassg casassg left a comment

Choose a reason for hiding this comment

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

lgtm, minus:

  • Switch to implement Pusher interface
  • Explicitly state dependencies
  • Remove GCS path for credentials from interface

The implementation details
- Define a custom Python function-based TFX component. It takes the following parameters from a previous component.
- The URI of the pushed model from TFX Pusher component.
- Requirements from Firebase ML (credential JSON file path, Firebase temporary-use GCS bucket). Please find more information from [Before you begin section](https://firebase.google.com/docs/ml/manage-hosted-models#before_you_begin) in the official Firebase document.
Copy link
Member

Choose a reason for hiding this comment

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

When using Kubernetes on GKE, you can use Workload identity to set up your pods to automatically auth as the user (see: https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity)

Otherwise, you can set it up using https://github.com/tensorflow/tfx/blob/master/tfx/orchestration/kubeflow/kubeflow_dag_runner.py#L78 (which is preferred over setting a GCS path)

@rcrowe-google
Copy link
Collaborator

lgtm, minus:

  • Switch to implement Pusher interface
  • Explicitly state dependencies
  • Remove GCS path for credentials from interface

@deep-diver please ping me when you have made these changes to the proposal.

@github-actions
Copy link
Contributor

Thanks for the PR! 🚀

Instructions: Approve using /lgtm and mark for automatic merge by using /merge.

@deep-diver
Copy link
Contributor Author

@casassg
can you please review the recent commits? I have included what has been mentioned, but I want to make sure everything is stated correctly.

@rcrowe-google
I will let you know once @casassg approves the changes! Thanks you :)

Copy link
Member

@casassg casassg left a comment

Choose a reason for hiding this comment

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

Lgtm! Ready to ship!

- Define a custom Python function-based TFX component. It takes the following parameters from a previous component.
- It should follow the standard Pusher's interface since this is another custom pusher.
- Additionally, it takes meta information to manage published model for Firebase ML such as `display name` and `tags`.
- Download saved TFLite model file by referncing the output from a previous component
Copy link
Member

Choose a reason for hiding this comment

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

*referencing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just fixed the typo. Thank you :)

@deep-diver
Copy link
Contributor Author

@rcrowe-google
I have addressed all the raised issues to the proposal! please review!

@rcrowe-google rcrowe-google merged commit 3e9e257 into tensorflow:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants