Skip to content

Mlkit fix date handling2 #391

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 27, 2020
Merged

Mlkit fix date handling2 #391

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fcc3b2d
Firebase ML Modify Operation Handling Code to match actual codes
ifielker Jan 23, 2020
5c923ce
apply database fix too
ifielker Jan 23, 2020
16815e2
fix create/update date handling
ifielker Jan 23, 2020
efb44ae
Merge branch 'mlkit' into mlkitFixDateHandling2
ifielker Jan 23, 2020
b68f8fc
Fix bad merge
ifielker Jan 23, 2020
bb0b146
conversion to millis
ifielker Jan 24, 2020
3fead5f
skip unrelated failing tests (until sync)
ifielker Jan 27, 2020
6e2c1eb
fix for review comments
ifielker Jan 27, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion firebase_admin/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
503: exceptions.UNAVAILABLE,
}


# See https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto
_RPC_CODE_TO_ERROR_CODE = {
1: exceptions.CANCELLED,
Expand All @@ -77,7 +78,6 @@
16: exceptions.UNAUTHENTICATED,
}


def _get_initialized_app(app):
if app is None:
return firebase_admin.get_app()
Expand Down
13 changes: 2 additions & 11 deletions firebase_admin/ml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@


import datetime
import numbers
import re
import time
import requests
Expand Down Expand Up @@ -246,20 +245,12 @@ def display_name(self, display_name):
@property
def create_time(self):
"""The time the model was created."""
seconds = self._data.get('createTime', {}).get('seconds')
if not isinstance(seconds, numbers.Number):
return None

return datetime.datetime.fromtimestamp(float(seconds))
return self._data.get('createTime', None)
Copy link
Contributor

@hiranya911 hiranya911 Jan 23, 2020

Choose a reason for hiding this comment

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

We generally expose timestamps as milliseconds since epoch values in Python. We might want to retain that behavior here as well. String timestamps are kind of awkward in Python (since it requires the user to handle the timezone and the format of the string).

firebase-admin-python/firebase_admin/_user_mgt.py

Lines 51 to 66 in 00cf1d3

def creation_timestamp(self):
""" Creation timestamp in milliseconds since the epoch.
Returns:
integer: The user creation timestamp in milliseconds since the epoch.
"""
return self._creation_timestamp
@property
def last_sign_in_timestamp(self):
""" Last sign in timestamp in milliseconds since the epoch.
Returns:
integer: The last sign in timestamp in milliseconds since the epoch.
"""
return self._last_sign_in_timestamp

What did we agree to in the API review for this?
Copy link
Contributor Author

@ifielker ifielker Jan 24, 2020

Choose a reason for hiding this comment

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

Millis. I changed it to do the conversion. Thanks.

@property
def update_time(self):
"""The time the model was last updated."""
seconds = self._data.get('updateTime', {}).get('seconds')
if not isinstance(seconds, numbers.Number):
return None

return datetime.datetime.fromtimestamp(float(seconds))
return self._data.get('updateTime', None)

@property
def validation_error(self):
Expand Down
52 changes: 19 additions & 33 deletions tests/test_ml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

"""Test cases for the firebase_admin.ml module."""

import datetime
import json
import pytest

Expand All @@ -27,25 +26,12 @@
PROJECT_ID = 'myProject1'
PAGE_TOKEN = 'pageToken'
NEXT_PAGE_TOKEN = 'nextPageToken'
CREATE_TIME_SECONDS = 1566426374
CREATE_TIME_SECONDS_2 = 1566426385
CREATE_TIME_JSON = {
'seconds': CREATE_TIME_SECONDS
}
CREATE_TIME_DATETIME = datetime.datetime.fromtimestamp(CREATE_TIME_SECONDS)
CREATE_TIME_JSON_2 = {
'seconds': CREATE_TIME_SECONDS_2
}
CREATE_TIME = '2020-01-21T20:44:27.392932Z'
CREATE_TIME_2 = '2020-01-21T21:44:27.392932Z'

UPDATE_TIME = '2020-01-21T22:45:29.392932Z'
UPDATE_TIME_2 = '2020-01-21T23:45:29.392932Z'

UPDATE_TIME_SECONDS = 1566426678
UPDATE_TIME_SECONDS_2 = 1566426691
UPDATE_TIME_JSON = {
'seconds': UPDATE_TIME_SECONDS
}
UPDATE_TIME_DATETIME = datetime.datetime.fromtimestamp(UPDATE_TIME_SECONDS)
UPDATE_TIME_JSON_2 = {
'seconds': UPDATE_TIME_SECONDS_2
}
ETAG = '33a64df551425fcc55e4d42a148795d9f25f89d4'
MODEL_HASH = '987987a98b98798d098098e09809fc0893897'
TAG_1 = 'Tag1'
Expand Down Expand Up @@ -130,8 +116,8 @@
CREATED_UPDATED_MODEL_JSON_1 = {
'name': MODEL_NAME_1,
'displayName': DISPLAY_NAME_1,
'createTime': CREATE_TIME_JSON,
'updateTime': UPDATE_TIME_JSON,
'createTime': CREATE_TIME,
Copy link
Contributor

@hiranya911 hiranya911 Jan 27, 2020

Choose a reason for hiding this comment

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

Can these 2 values ever be not set in response? At least the parsing logic assumes so. Might want to add a test case for it.
Copy link
Contributor Author

@ifielker ifielker Jan 27, 2020

Choose a reason for hiding this comment

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

They will always be set in the response.
'updateTime': UPDATE_TIME,
'state': MODEL_STATE_ERROR_JSON,
'etag': ETAG,
'modelHash': MODEL_HASH,
Expand All @@ -142,17 +128,17 @@
LOCKED_MODEL_JSON_1 = {
'name': MODEL_NAME_1,
'displayName': DISPLAY_NAME_1,
'createTime': CREATE_TIME_JSON,
'updateTime': UPDATE_TIME_JSON,
'createTime': CREATE_TIME,
'updateTime': UPDATE_TIME,
'tags': TAGS,
'activeOperations': [OPERATION_NOT_DONE_JSON_1]
}

LOCKED_MODEL_JSON_2 = {
'name': MODEL_NAME_1,
'displayName': DISPLAY_NAME_2,
'createTime': CREATE_TIME_JSON_2,
'updateTime': UPDATE_TIME_JSON_2,
'createTime': CREATE_TIME_2,
'updateTime': UPDATE_TIME_2,
'tags': TAGS_2,
'activeOperations': [OPERATION_NOT_DONE_JSON_1]
}
Expand Down Expand Up @@ -183,8 +169,8 @@
FULL_MODEL_ERR_STATE_LRO_JSON = {
'name': MODEL_NAME_1,
'displayName': DISPLAY_NAME_1,
'createTime': CREATE_TIME_JSON,
'updateTime': UPDATE_TIME_JSON,
'createTime': CREATE_TIME,
'updateTime': UPDATE_TIME,
'state': MODEL_STATE_ERROR_JSON,
'etag': ETAG,
'modelHash': MODEL_HASH,
Expand All @@ -194,8 +180,8 @@
FULL_MODEL_PUBLISHED_JSON = {
'name': MODEL_NAME_1,
'displayName': DISPLAY_NAME_1,
'createTime': CREATE_TIME_JSON,
'updateTime': UPDATE_TIME_JSON,
'createTime': CREATE_TIME,
'updateTime': UPDATE_TIME,
'state': MODEL_STATE_PUBLISHED_JSON,
'etag': ETAG,
'modelHash': MODEL_HASH,
Expand Down Expand Up @@ -364,8 +350,8 @@ def test_model_success_err_state_lro(self):
model = ml.Model.from_dict(FULL_MODEL_ERR_STATE_LRO_JSON)
assert model.model_id == MODEL_ID_1
assert model.display_name == DISPLAY_NAME_1
assert model.create_time == CREATE_TIME_DATETIME
assert model.update_time == UPDATE_TIME_DATETIME
assert model.create_time == CREATE_TIME
assert model.update_time == UPDATE_TIME
assert model.validation_error == VALIDATION_ERROR_MSG
assert model.published is False
assert model.etag == ETAG
Expand All @@ -379,8 +365,8 @@ def test_model_success_published(self):
model = ml.Model.from_dict(FULL_MODEL_PUBLISHED_JSON)
assert model.model_id == MODEL_ID_1
assert model.display_name == DISPLAY_NAME_1
assert model.create_time == CREATE_TIME_DATETIME
assert model.update_time == UPDATE_TIME_DATETIME
assert model.create_time == CREATE_TIME
assert model.update_time == UPDATE_TIME
assert model.validation_error is None
assert model.published is True
assert model.etag == ETAG
Expand Down