-
Notifications
You must be signed in to change notification settings - Fork 316
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
chore: End-to-End Tabular System Test #610
Conversation
tests/system/aiplatform/test_resources/california_housing_training_script.py
Show resolved
Hide resolved
yield shared_state | ||
|
||
@pytest.fixture() | ||
def prepare_staging_bucket(self, shared_state): |
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.
It seems like it would be more reusable to create a base class that includes all these fixtures, if pytest support this. It would make creating new tests avoid this boilerplate.
Separating the creation of the bucket and uploading of the test dataset would make it easier to reuse.
storage_client = storage.Client(project=_PROJECT) | ||
shared_state["storage_client"] = storage_client | ||
|
||
bucket = storage_client.create_bucket(new_staging_bucket, location=_LOCATION) |
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.
What are the advantages of creating a new bucket for each integration test vs using a common bucket and creating a new directory?
@sasha-gitg I split out re-usable logic to a base One last TODO before merging is to have |
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.
Thanks Vinny, this looks great! Left a few minor comments.
Summary of Changes
CustomTrainingJob
with inner script andAutoMLTabularTrainingJob
system-3.8
for pull requests like this one (editing of system tests)Fixes b/172263198 🦕