Integration tests for Firebase ML #394

ifielker · 2020-01-27T23:06:13Z

Integration tests for Firebase ML

hiranya911

Looks pretty good. Just a few comments on style.

integration/test_ml.py

hiranya911

Looks pretty good. Just a few nits. Also consider using random identifiers for test resources.

integration/test_ml.py

hiranya911

Looks great! Just minor nits and one somewhat big proposal :)

hiranya911 · 2020-01-30T00:07:49Z

integration/test_ml.py

+# limitations under the License.
+
+"""Integration tests for firebase_admin.ml module."""
+import re


Nit: Organize in alphabetical order (I thought pylint was supposed to check this. I'll check why it's not getting verified.)

hiranya911 · 2020-01-30T00:11:52Z

integration/test_ml.py

+
+
+NAME_ONLY_ARGS = {
+    'display_name': 'TestModel123_{0}'.format(random.randint(1111, 9999))


Perhaps define a helper:

import random import string def _random_identifier(prefix): suffix = ''.join([random.choice(string.ascii_letters + string.digits) for n in xrange(8)]) return '{0}_{1}'.format(prefix, suffix)

PS: Just realized that this is only important if display name uniqueness is enforced by the ML API. If not constant identifiers are fine. Sorry, I forgot to ask earlier.

Uniqueness is enforced. Done. But I changed to range because Python3 doesn't have xrange.

hiranya911 · 2020-01-30T00:14:37Z

integration/test_ml.py

+    assert model.etag is not None
+
+
+def check_model_format(model, has_model_format, validation_error):


Set has_model_format=False, validation_error=None and simplify the call-site.

hiranya911 · 2020-01-30T00:19:17Z

integration/test_ml.py

+@pytest.fixture
+def saved_model_dir(keras_model):
+    assert _TF_ENABLED
+    # different versions have different model conversion capability


This comment probably should be pushed down a couple of lines.

Also add a comment explaining the requirement that child directory must not already exist.

hiranya911 · 2020-01-30T00:21:58Z

integration/test_ml.py

+    created_model = ml.create_model(model)
+
+    try:
+        assert created_model.model_id is not None


check_model(created_model, {'display_name': model_name}

hiranya911 · 2020-01-30T00:22:41Z

integration/test_ml.py

+
+    # Validate the conversion by creating a model
+    model_format = ml.TFLiteFormat(model_source=source)
+    model = ml.Model(display_name="KerasModel1", model_format=model_format)


Should probably use a random identifier here too (I'm assuming display_name is a unique key in this API).

hiranya911 · 2020-01-30T00:29:50Z

integration/test_ml.py

+NAME_ONLY_ARGS = {
+    'display_name': 'TestModel123_{0}'.format(random.randint(1111, 9999))
+}
+NAME_ONLY_ARGS_UPDATED = {


Now that we have done the refactor, I wonder if it makes sense to define each of these as a fixture.

@pytest.fixture def name_only_model(): args = { 'display_name': _random_identifier('TestModel') } model = ml.create_model(ml.Model(**args)) yield model, args _clean_up_model(model) @pytest.fixture(scope='module') def tflite_format(): # Create the model format from the file and return # Thought defining this bit as a module-scoped fixture will speed things up a bit @pytest.fixture def full_model(tflite_format): # Here we reference the tflite_format fixture args = { 'display_name': _random_identifier('TestFullModel'), 'tags': ['test1', 'test'2], 'model_format': tflite_format, } model = ml.create_model(ml.Model(**args)) yield model, args _clean_up_model(model) def test_create_simple_model(name_only_model): model, args = name_only_model check_model(model, args)

That simplifies the call-site significantly, and makes sure multiple calls to the same fixture don't reuse identifiers.

We started with something very similar to this. I think we've done enough refactoring now.

hiranya911 · 2020-01-30T00:46:01Z

integration/test_ml.py

+    file_name = args.get('file_name')
+    if file_name:
+        file_path = testutils.resource_filename(file_name)
+        source = ml.TFLiteGCSModelSource.from_tflite_model_file(file_path)


I assume each invocation of this makes an GCS upload? Do we need to clean them up at some point?

No. They overwrite and they are not randomized. So there should only ever be 3 files.

hiranya911 · 2020-01-30T00:47:10Z

integration/test_ml.py

+
+@pytest.fixture
+def model_list():
+    ml_model_1 = ml.Model(display_name="TestModel123")


Use random IDs.

Guess, I should've asked first. Does the ML API enforce uniqueness in display name? If not we don't really need any randomization.

hiranya911

LGTM with a comment.

hiranya911 · 2020-01-30T18:54:10Z

integration/test_ml.py

@@ -34,27 +35,34 @@
    _TF_ENABLED = False


+def _random_identifier(prefix):
+    #pylint: disable=unused-variable


for _ in range(8) and remove the ignore directive.

* 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 <[email protected]> Co-authored-by: Kevin Cheung <[email protected]>

Integration tests for Firebase ML

7a7a2a5

ifielker assigned hiranya911 Jan 27, 2020

hiranya911 suggested changes Jan 28, 2020

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Jan 28, 2020

addressed review comments

99ccb6d

ifielker assigned hiranya911 Jan 29, 2020

hiranya911 suggested changes Jan 29, 2020

View reviewed changes

review suggestions #2

0bc734c

hiranya911 suggested changes Jan 30, 2020

View reviewed changes

review suggestions #3

ae9527f

hiranya911 approved these changes Jan 30, 2020

View reviewed changes

hiranya911 removed their assignment Jan 30, 2020

ifielker merged commit 0b70687 into mlkit Jan 30, 2020

ifielker deleted the mlkitInteg branch January 30, 2020 19:48



		NAME_ONLY_ARGS = {
		'display_name': 'TestModel123_{0}'.format(random.randint(1111, 9999))

		assert model.etag is not None


		def check_model_format(model, has_model_format, validation_error):

Integration tests for Firebase ML #394

Integration tests for Firebase ML #394

Uh oh!

Conversation

ifielker commented Jan 27, 2020

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!