-
Notifications
You must be signed in to change notification settings - Fork 350
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments. Thanks Sara!
@@ -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 |
There was a problem hiding this comment.
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.
if isinstance(datasource, _datasources.DatasourceImportable): | ||
dataset_obj._import_and_wait(datasource) | ||
dataset_obj._import_and_wait(datasource, import_request_timeout=None) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
Adds e2e test for AutoML Forecasting and unit test for `TimeSeriesDataset`. Also adds `create_request_timeout` to `TimeSeriesDataset`, which #1099 seems to have missed. --- Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-aiplatform/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary) Fixes #<issue_number_goes_here> 🦕
Addresses b/225964107 🦕
I added a
timeout
arg and relevant tests to these methods:Model.upload
Model.deploy
PipelineJob.submit
*TrainingJob.run
CustomJob.run
HyperparameterTuningJob.run
BatchPredictionJob.create
*Dataset.create
*Dataset.import_data
Tensorboard.create
TensorboardExperiment.create
TensorboardRun.create
Featurestore.create
Featurestore.create_entity_type
Featurestore.update
Featurestore.batch_serve_to_bq
Featurestore.batch_serve_to_gcs
Featurestore.batch_serve_to_df
EntityType.create
EntityType.update
EntityType.read
EntityType.create_feature
EntityType.batch_create_features
EntityType.ingest_from_bq
EntityType.ingest_from_gcs
EntityType.ingest_from_df
Feature.create
Feature.update
For most of these methods, I gave the timeout arg a name specific to the method (i.e.
upload_request_timeout
) to differentiate the timeout of initiating the request (what this PR applies to) vs. the timeout of the underlying job.