Skip to content

Implement initial Dataset class #31

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

Jump to bottom
Merged
merged 6 commits into from
Oct 20, 2020
Merged

Implement initial Dataset class #31

merged 6 commits into from
Oct 20, 2020

Conversation

@vinnysenthil
Copy link
Contributor

@vinnysenthil vinnysenthil commented Oct 20, 2020

Summary of changes:

  • Implement Dataset class with following methods
    • __init__
    • _create
    • _import
    • create
    • import_data
    • export_data
  • Ten unit tests covering 98% of Dataset class
  • Add two helper functions to utils.py — validate_id and validate_string_list
  • Add an error when project is not initialized or set in credentials

Fixes b/169781839

@vinnysenthil
Dataset class (#1)
eb601cd 
* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2020
sasha-gitg
sasha-gitg approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@sasha-gitg sasha-gitg left a comment

Choose a reason for hiding this comment

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

Looks great! I really like how you used pytest fixtures for patching.

dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
google/cloud/aiplatform/datasets.py
if not source:
return dataset_obj

return dataset_obj.import_data(
Copy link
Contributor

@dizcology dizcology Oct 20, 2020

Choose a reason for hiding this comment

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

This would return an LRO, while in the "no data import" case, we wait until the create_dataset LRO completes. We need to document clearly (e.g. in the docstring) this inconsistent behavior.

Copy link
Contributor Author

@vinnysenthil vinnysenthil Oct 20, 2020

Choose a reason for hiding this comment

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

Yeah these multiple LROs under-the-hood can be misleading to the user. I'm adding this discussion to b/171275584

dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
tests/unit/aiplatform/test_datasets.py
@pytest.fixture
def get_dataset_mock(self):
with patch.object(DatasetServiceClient, "get_dataset") as get_dataset_mock:
get_dataset_mock.return_value = GapicDataset(
Copy link
Contributor

@dizcology dizcology Oct 20, 2020

Choose a reason for hiding this comment

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

this can become difficult to maintain to make sure the mocks return correct responses. we should look into replays.

Copy link
Contributor Author

@vinnysenthil vinnysenthil Oct 20, 2020

Choose a reason for hiding this comment

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

Captured in b/171334022

dizcology
dizcology reviewed Oct 20, 2020
View reviewed changes
tests/unit/aiplatform/test_datasets.py
reload(aip)

@pytest.fixture
def get_dataset_mock(self):
Copy link
Contributor

@dizcology dizcology Oct 20, 2020

Choose a reason for hiding this comment

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

perhaps move the mock fixtures into conftest.py so they could be more easily reused.

Copy link
Contributor Author

@vinnysenthil vinnysenthil Oct 20, 2020

Choose a reason for hiding this comment

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

Captured in b/171333554

@dizcology dizcology added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
@vinnysenthil vinnysenthil added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 20, 2020
@vinnysenthil vinnysenthil merged commit d65512e into googleapis:dev Oct 20, 2020
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
970ddf2 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
308370e 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
ece29f2 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
dfd126e 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Nov 30, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
31fb3d0 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
dizcology pushed a commit to dizcology/python-aiplatform that referenced this pull request Dec 22, 2020
@vinnysenthil @dizcology
Implement initial Dataset class (googleapis#31)
ab652b6 
* Dataset class (#1)

* Add global_config unset project error

* Add two validation functions to utils

* Implement initial Dataset class

* Lint utils.py

* Address reviewer comments + remove aip alias

* Change re.Match to typing.Match

* Lint with Py 3.8

* Address flake8 errors, remove unused vars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dizcology dizcology dizcology left review comments

@sasha-gitg sasha-gitg sasha-gitg approved these changes

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vinnysenthil @dizcology @sasha-gitg @yoshi-kokoro