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

feat: add timeout arg across SDK #1099

Merged
merged 24 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fa123c2
feat: add timeout arg and tests to model upload
sararob Mar 22, 2022
d7a387b
feat: add timeout arg and tests to dataset create method
sararob Mar 22, 2022
5952489
feat: add tiemout arg to dataset import and tensorboard create
sararob Mar 22, 2022
de1f9ee
Merge branch 'main' into add-timeout-arg
sararob Mar 22, 2022
a696ff4
Update system tests with timeout arg
sararob Mar 22, 2022
1572273
rename timeout arg with method name and update tests
sararob Mar 23, 2022
5bf72f4
add deploy_request_timeout to Model deploy
sararob Mar 23, 2022
9d383b2
add create_timeout_request arg to pipeline job run and submit
sararob Mar 23, 2022
dd3d57b
add timeout arg and tests to training_jobs
sararob Mar 23, 2022
a4a852c
add timeout arg tests for training_jobs
sararob Mar 24, 2022
8530598
update system tests with timeout arg
sararob Mar 24, 2022
8cc46b0
add timeout arg tests and run linter
sararob Mar 24, 2022
77994c3
add timeout arg and tests to tensorboard
sararob Mar 24, 2022
e72c07a
add timeout arg and tests to featurestore
sararob Mar 24, 2022
7a28034
Merge branch 'main' into add-timeout-arg
sararob Mar 24, 2022
3e5dd00
fix failing tests
sararob Mar 25, 2022
89f4815
Merge branch 'main' into add-timeout-arg
sararob Mar 25, 2022
2f8d67f
update system tests with timeout arg
sararob Mar 25, 2022
3d211dd
fix broken tests and run linter
sararob Mar 25, 2022
9e4163b
resolve conflicts, move timeout arg to end of signature, shorten docs…
sararob Mar 31, 2022
0b30635
update handling of import_request_timeout arg
sararob Mar 31, 2022
bd4df2e
update timeout arg in tests and run linter
sararob Apr 1, 2022
adf6694
Merge branch 'main' into add-timeout-arg
sararob Apr 1, 2022
12bb2f9
finish moving timeout arg to end of function signatures
sararob Apr 1, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions google/cloud/aiplatform/datasets/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def create(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec_key_name: Optional[str] = None,
create_request_timeout: Optional[float] = None,
sararob marked this conversation as resolved.
Show resolved Hide resolved
sync: bool = True,
) -> "_Dataset":
"""Creates a new dataset and optionally imports data into dataset when
Expand Down Expand Up @@ -199,6 +200,10 @@ def create(
If set, this Dataset and all sub-resources of this Dataset will be secured by this key.

Overrides encryption_spec_key_name set in aiplatform.init.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
Copy link
Member

Choose a reason for hiding this comment

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

Preference to reduce this docstring to The timeout for the create request in seconds. throughout.

to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand Down Expand Up @@ -239,6 +244,7 @@ def create(
encryption_spec=initializer.global_config.get_encryption_spec(
encryption_spec_key_name=encryption_spec_key_name
),
create_request_timeout=create_request_timeout,
sync=sync,
)

Expand All @@ -257,6 +263,7 @@ def _create_and_import(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec: Optional[gca_encryption_spec.EncryptionSpec] = None,
create_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "_Dataset":
"""Creates a new dataset and optionally imports data into dataset when
Expand Down Expand Up @@ -309,6 +316,10 @@ def _create_and_import(
resource is created.

If set, this Dataset and all sub-resources of this Dataset will be secured by this key.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand All @@ -328,6 +339,7 @@ def _create_and_import(
request_metadata=request_metadata,
labels=labels,
encryption_spec=encryption_spec,
create_request_timeout=create_request_timeout,
)

_LOGGER.log_create_with_lro(cls, create_dataset_lro)
Expand All @@ -344,17 +356,20 @@ def _create_and_import(
)

# Import if import datasource is DatasourceImportable
# import_request_timeout is None since user is issuing a single request with create and import
if isinstance(datasource, _datasources.DatasourceImportable):
dataset_obj._import_and_wait(datasource)
dataset_obj._import_and_wait(datasource, import_request_timeout=None)
Copy link
Member

Choose a reason for hiding this comment

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

Preference to handle this as a default in _import_and_wait instead of passing None 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.

In this case should create() accept both create_request_timeout and import_request_timeout parameters?


return dataset_obj

def _import_and_wait(self, datasource):
def _import_and_wait(self, datasource, import_request_timeout):
Copy link
Member

Choose a reason for hiding this comment

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

Preference to use this chance to include a docstring and type hints.

_LOGGER.log_action_start_against_resource(
"Importing", "data", self,
)

import_lro = self._import(datasource=datasource)
import_lro = self._import(
datasource=datasource, import_request_timeout=import_request_timeout
)

_LOGGER.log_action_started_against_resource_with_lro(
"Import", "data", self.__class__, import_lro
Expand All @@ -375,6 +390,7 @@ def _create(
request_metadata: Sequence[Tuple[str, str]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec: Optional[gca_encryption_spec.EncryptionSpec] = None,
create_request_timeout: Optional[float] = None,
) -> operation.Operation:
"""Creates a new managed dataset by directly calling API client.

Expand Down Expand Up @@ -417,6 +433,10 @@ def _create(
resource is created.

If set, this Dataset and all sub-resources of this Dataset will be secured by this key.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
Returns:
operation (Operation):
An object representing a long-running operation.
Expand All @@ -431,11 +451,16 @@ def _create(
)

return api_client.create_dataset(
parent=parent, dataset=gapic_dataset, metadata=request_metadata
parent=parent,
dataset=gapic_dataset,
metadata=request_metadata,
timeout=create_request_timeout,
)

def _import(
self, datasource: _datasources.DatasourceImportable,
self,
datasource: _datasources.DatasourceImportable,
import_request_timeout: Optional[float] = None,
sararob marked this conversation as resolved.
Show resolved Hide resolved
) -> operation.Operation:
"""Imports data into managed dataset by directly calling API client.

Expand All @@ -448,7 +473,9 @@ def _import(
An object representing a long-running operation.
"""
return self.api_client.import_data(
name=self.resource_name, import_configs=[datasource.import_data_config]
name=self.resource_name,
import_configs=[datasource.import_data_config],
timeout=import_request_timeout,
)

@base.optional_sync(return_input_arg="self")
Expand All @@ -457,6 +484,7 @@ def import_data(
gcs_source: Union[str, Sequence[str]],
import_schema_uri: str,
data_item_labels: Optional[Dict] = None,
import_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "_Dataset":
"""Upload data to existing managed dataset.
Expand Down Expand Up @@ -491,6 +519,10 @@ def import_data(
labels specified inside index file referenced by
``import_schema_uri``,
e.g. jsonl file.
import_request_timeout (float):
Optional. The timeout for initiating this import request in seconds. Note:
this does not set the timeout on the underlying import job, only on the time
to initiate the import request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand All @@ -507,7 +539,9 @@ def import_data(
data_item_labels=data_item_labels,
)

self._import_and_wait(datasource=datasource)
self._import_and_wait(
datasource=datasource, import_request_timeout=import_request_timeout
)
return self

# TODO(b/174751568) add optional sync support
Expand Down
6 changes: 6 additions & 0 deletions google/cloud/aiplatform/datasets/image_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def create(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec_key_name: Optional[str] = None,
create_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "ImageDataset":
"""Creates a new image dataset and optionally imports data into dataset
Expand Down Expand Up @@ -117,6 +118,10 @@ def create(
If set, this Dataset and all sub-resources of this Dataset will be secured by this key.

Overrides encryption_spec_key_name set in aiplatform.init.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand Down Expand Up @@ -158,5 +163,6 @@ def create(
encryption_spec=initializer.global_config.get_encryption_spec(
encryption_spec_key_name=encryption_spec_key_name
),
create_request_timeout=create_request_timeout,
sync=sync,
)
6 changes: 6 additions & 0 deletions google/cloud/aiplatform/datasets/tabular_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def create(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec_key_name: Optional[str] = None,
create_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "TabularDataset":
"""Creates a new tabular dataset.
Expand Down Expand Up @@ -98,6 +99,10 @@ def create(
If set, this Dataset and all sub-resources of this Dataset will be secured by this key.

Overrides encryption_spec_key_name set in aiplatform.init.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand Down Expand Up @@ -138,6 +143,7 @@ def create(
encryption_spec=initializer.global_config.get_encryption_spec(
encryption_spec_key_name=encryption_spec_key_name
),
create_request_timeout=create_request_timeout,
sync=sync,
)

Expand Down
6 changes: 6 additions & 0 deletions google/cloud/aiplatform/datasets/text_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def create(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec_key_name: Optional[str] = None,
create_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "TextDataset":
"""Creates a new text dataset and optionally imports data into dataset
Expand Down Expand Up @@ -124,6 +125,10 @@ def create(
If set, this Dataset and all sub-resources of this Dataset will be secured by this key.

Overrides encryption_spec_key_name set in aiplatform.init.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand Down Expand Up @@ -165,5 +170,6 @@ def create(
encryption_spec=initializer.global_config.get_encryption_spec(
encryption_spec_key_name=encryption_spec_key_name
),
create_request_timeout=create_request_timeout,
sync=sync,
)
6 changes: 6 additions & 0 deletions google/cloud/aiplatform/datasets/video_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def create(
request_metadata: Optional[Sequence[Tuple[str, str]]] = (),
labels: Optional[Dict[str, str]] = None,
encryption_spec_key_name: Optional[str] = None,
create_request_timeout: Optional[float] = None,
sync: bool = True,
) -> "VideoDataset":
"""Creates a new video dataset and optionally imports data into dataset
Expand Down Expand Up @@ -117,6 +118,10 @@ def create(
If set, this Dataset and all sub-resources of this Dataset will be secured by this key.

Overrides encryption_spec_key_name set in aiplatform.init.
create_request_timeout (float):
Optional. The timeout for initiating this create request in seconds. Note:
this does not set the timeout on the underlying create job, only on the time
to initiate the create request.
sync (bool):
Whether to execute this method synchronously. If False, this method
will be executed in concurrent Future and any downstream object will
Expand Down Expand Up @@ -158,5 +163,6 @@ def create(
encryption_spec=initializer.global_config.get_encryption_spec(
encryption_spec_key_name=encryption_spec_key_name
),
create_request_timeout=create_request_timeout,
sync=sync,
)
Loading