-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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: New Queued resources Samples: Create, Create Spot, Get, Delete operations + tests #12716
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 9 region tags.
This comment is generated by snippet-bot.
|
tpu/queued_resources_delete.py
Outdated
except Exception as e: | ||
print(f"Error deleting resource: {e}") |
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.
I think in this case we can raise e
, so we don't ignore proper issues.
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.
I simplified this code block.
tpu/test_queued_resources.py
Outdated
except Exception: | ||
continue |
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.
Maybe print the error, so it's easier to debug any potential issues.
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.
Done
tpu/test_queued_resources.py
Outdated
except Exception: | ||
continue |
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.
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.
Done
tpu/test_queued_resources.py
Outdated
] | ||
|
||
|
||
def clean_resource() -> 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.
Add some comment why you also delete TPU here, that it's liked to the queued resource etc.
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.
Added expanded comment
Added new Samples: Force Delete, Network, Startup Script. Updated test approach.
Description
📑Created a full set of Queued Resources Samples:
❗Issue with Deleting operation in
google.cloud.tpu_v2alpha1
The delete operation in
queued_resources_delete.py
have issue here with thisblock of code.
Issues Encountered:
Resource Successfully Deleted but we got TypeError: After invoking op.result() the resource is indeed successfully deleted. However, despite the deletion being successful, the function throws a
TypeError: Could not convert Any to QueuedResource.
This error suggests that the API response could not be properly deserialized or converted into a QueuedResource object, even though the deletion itself completed as expected.
The code should not throw a TypeError when the resources are successfully deleted. We cannot avoid using op.result() because it guarantees that an Exception will be raised when attempting to delete a resource in a status where deletion is not possible. This behavior ensures proper error handling in situations where the resource cannot be deleted, but the current implementation incorrectly throws a TypeError even when deletion is successful.
This is precisely why I had to use two separate except blocks in the code.
Checklist
nox -s py-3.9
(see Test Environment Setup)nox -s lint
(see Test Environment Setup)