Firebase ML Kit Get Model API implementation #326

ifielker · 2019-08-19T18:12:34Z

Firebase ML Kit Get Model API implementation

hiranya911

Thanks for keeping changes small. This looks mostly good. Just a few things pointed out for improvement. Additionally:

Fix the lint errors
Remove line 19 of the .travis.yml file for now (RTDB team is working on the test failures)
Edit the PR title and type in something a bit more descriptive (makes it easier to compile release notes and stuff later)

firebase_admin/mlkit.py

hiranya911 · 2019-08-19T18:30:08Z

firebase_admin/mlkit.py

 class _MLKitService(object):
    """Firebase MLKit service."""

-    BASE_URL = 'https://mlkit.googleapis.com'
-    PROJECT_URL = 'https://mlkit.googleapis.com/projects/{0}/'
+    BASE_URL = 'https://mlkit.googleapis.com/v1beta1/'


Doesn't seem to be used in this module. Perhaps move to the tests module, where it's being referenced?

hiranya911 · 2019-08-19T18:32:32Z

firebase_admin/mlkit.py

+        self._project_url = _MLKitService.PROJECT_URL.format(project_id)
+        self._client = _http_client.JsonHttpClient(credential=app.credential.get_credential())
+
+    def _request(self, method, urlpath, **kwargs):


This seems to be just appending the path to the base/project URL. I'd suggest passing the base_url parameter to the JsonHttpClient constructor and getting rid of this. You can just call the client directly as:

self._client.body(method, urlpath)

hiranya911 · 2019-08-19T18:33:32Z

firebase_admin/mlkit.py

+        try:
+            return self._request('get', 'models/{0}'.format(model_id))
+        except requests.exceptions.RequestException as error:
+            raise _utils.handle_requests_error(error)


Since this is a OnePlatform API, it's better to call _utils.handle_platform_error_from_requests(error), which knows how to parse OP errors.

hiranya911 · 2019-08-19T18:35:16Z

tests/test_mlkit.py

+        testutils.cleanup_apps()
+
+    @staticmethod
+    def check_error(err, errType, msg):


s/errType/err_type

hiranya911 · 2019-08-19T18:38:59Z

firebase_admin/mlkit.py

+        if not model_id:
+            raise ValueError('Model Id is required for GetModel.')
+        if not isinstance(model_id, six.string_types):
+            raise TypeError('Model Id must be a string.')


hiranya911 · 2019-08-19T18:44:09Z

firebase_admin/mlkit.py

+        return self._client.body(method, url=self._project_url + urlpath, **kwargs)
+
+    def get_model(self, model_id):
+        if not model_id:


This can be removed. If model_id is None, your type check will catch it. If it's empty your regex check will catch it.

hiranya911 · 2019-08-19T18:47:02Z

tests/test_mlkit.py

+        _, recorder = self._instrument_mlkit_service(status=404, payload=_ERROR_RESPONSE)
+        with pytest.raises(exceptions.NotFoundError) as err:
+            mlkit.get_model(MODEL_ID_1)
+        self.check_firebase_error(err.value, ERROR_STATUS, ERROR_CODE)


Also assert the error message. If the OP error got parsed correctly, error message gets set to the error.message field of the response.

hiranya911 · 2019-08-19T18:47:45Z

tests/test_mlkit.py

+            with pytest.raises(ValueError):
+                mlkit.get_model(MODEL_ID_1, app)
+        testutils.run_without_project_id(evaluate)
+


Remove redundant empty lines.

hiranya911

LGTM with a couple of suggestions 👍

hiranya911 · 2019-08-19T20:08:11Z

firebase_admin/mlkit.py

+                'Project ID is required to access MLKit service. Either set the '
+                'projectId option, or use service account credentials.')
+        self._project_url = _MLKitService.PROJECT_URL.format(project_id)
+        self._client = _http_client.JsonHttpClient(credential=app.credential.get_credential())


Also pass base_url=self._project_url to the constructor

hiranya911 · 2019-08-19T20:10:06Z

firebase_admin/mlkit.py

+        try:
+            return self._client.body(
+                'get',
+                url=self._project_url + 'models/{0}'.format(model_id))


Once you pass base_url the constructor, you don't have to do this concat here. Just pass the path modles/... portion as the url.

* Introduced the exceptions module (#296) * Added the exceptions module * Cleaned up the error handling logic; Added tests * Updated docs; Fixed some typos * Migrating FCM Send APIs to the New Exceptions (#297) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated remaining messaging APIs to new error types (#298) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Introducing TokenSignError to represent custom token creation errors (#302) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Raising FirebaseError from create_session_cookie() API (#306) * Migrated FCM send APIs to the new error handling regime * Moved error parsing logic to _utils * Refactored OP error handling code * Fixing a broken test * Added utils for handling googleapiclient errors * Added tests for new error handling logic * Updated public API docs * Fixing test for python3 * Cleaning up the error code lookup code * Cleaning up the error parsing APIs * Cleaned up error parsing logic; Updated docs * Migrated the FCM IID APIs to the new error types * Migrated custom token API to new error types * Migrated create cookie API to new error types * Improved error message computation * Refactored the shared error handling code * Fixing lint errors * Renamed variable for clarity * Introducing UserNotFoundError type (#309) * Added UserNotFoundError type * Fixed some lint errors * Some formatting updates * Updated docs and tests * New error handling support in create/update/delete user APIs (#311) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling improvements in email action link APIs (#312) * New error handling support in create/update/delete user APIs * Fixing some lint errors * Error handling update in email action link APIs * Project management API migrated to new error types (#314) * Error handling updated for remaining user_mgt APIs (#315) * Error handling updated for remaining user_mgt APIs * Removed unused constants * Migrated token verification APIs to new exception types (#317) * Migrated token verification APIs to new error types * Removed old AuthError type * Added new exception types for revoked tokens * Migrated the db module to the new exception types (#318) * Migrating db module to new exception types * Error handling for transactions * Updated integration tests * Restoring the old txn abort behavior * Updated error type in snippet * Added comment * Adding a few overlooked error types (#319) * Adding some missing error types * Updated documentation * Removing the ability to delete user properties by passing None (#320) * Adding beginning of _MLKitService (#323) * Adding beginning of _MLKitService * Added License and Docstring * Firebase ML Kit Get Model API implementation (#326) * added GetModel * Added tests for get_model * Firebase ML Kit Delete Model API implementation (#327) * implement delete model * Firebase ML Kit List Models API implementation (#331) * implemented list models plus tests * Implementation of Model, ModelFormat, TFLiteModelSource and subclasses (#335) * Implementation of Model, ModelFormat, ModelSource and subclasses * Firebase ML Kit Create Model API implementation (#337) * create model plus long running operation handling * Model.wait_for_unlocked * Firebase ML Kit Update Model API implementation (#343) * Firebase ML Kit Create Model API implementation * Firebase ML Kit Publish and Unpublish Implementation (#345) * Firebase ML Kit Publish and Unpublish Implementation * Firebase ML Kit TFLiteGCSModelSource.from_tflite_model implementation and conversion helpers (#346) * Firebase ML Kit TFLiteGCSModelSource.from_tflite_model implementation * support for tensorflow lite conversion helpers (version 1.x) * Quick pass at filling in missing docstrings (#367) * Quick pass at filling in missing docstrings * More punctuation * Modify Operation Handling to not require a name for Done Operations (#371) * Firebase ML Kit Modify Operation Handling to not require a name for Done Operations * Adding support for TensorFlow 2.x * rename from mlkit to ml (#373) * Adding File naming capability to from_saved_model and from_keras_model. (#375) adding File naming capability for ModelSource * Firebase ML Modify Operation Handling Code to match rpc codes not html codes (#390) * Firebase ML Modify Operation Handling Code to match actual codes * apply database fix too * Mlkit fix date handling2 (#391) * Fix create/update date handling * Skip unrelated failing tests (until sync) * Firebase Ml Fix upload file naming (#392) * Fix File Naming * Integration tests for Firebase ML (#394) * Integration tests for Firebase ML * Fixing lint errors for Py3 (#401) * Fixing lint errors for Py3 * Removed dependency on six * Fixing a couple of merge errors * Modifying operation handling to support backend changes (#423) * modifying operation handling to support backend changes * Firebase ML Changing service endpoint (#421) * Mlkit add headers (#445) * add Headers * fixed test (#448) * Adding tensorflow and keras so we don't skip tests (#449) * Adding tensorflow and keras so we don't skip tests * Add additional instructions for integration tests for ml Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>

Ingrid Fielker and others added 2 commits August 15, 2019 15:39

added GetModel

723ef2d

Added tests for get_model

976bf1e

ifielker requested a review from hiranya911 August 19, 2019 18:13

ifielker assigned hiranya911 Aug 19, 2019

hiranya911 suggested changes Aug 19, 2019

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Aug 19, 2019

ifielker added 2 commits August 19, 2019 15:05

fixed lint error

b47e70b

fixed issues from reviewer comments

95e7a7a

ifielker assigned hiranya911 Aug 19, 2019

ifielker added 2 commits August 19, 2019 15:54

fixed lint

aa263ac

more lint fixes

757cbc8

ifielker changed the title ~~Mlkit 2~~ Firebase ML Kit Get Model API implementation Aug 19, 2019

hiranya911 approved these changes Aug 19, 2019

View reviewed changes

hiranya911 removed their assignment Aug 19, 2019

fixed url handling

b688250

ifielker merged commit 65f64c0 into mlkit Aug 19, 2019

ifielker deleted the mlkit-2 branch August 19, 2019 20:28

Firebase ML Kit Get Model API implementation #326

Firebase ML Kit Get Model API implementation #326

Uh oh!

Conversation

ifielker commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ifielker commented Aug 19, 2019 •

edited

Loading