Skip to content

Added delete_engine_wait function that completes once an engine is completely deleted #129

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Added delete_engine_wait function that completes once an engine is completely deleted #129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8fefdc1
added delete_engine_wait function that completes once an engine is de…
Jul 6, 2023
9a3fe7d
added engine states enum, updated create and delete engine wait API t…
Jul 7, 2023
612288d
modified the delete_engine_wait test to check the engine gets deleted
Jul 7, 2023
9ac5df6
converted integration tests for Test API to unit tests
Jul 18, 2023
8e83614
removed an empty line
Jul 18, 2023
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
32 changes: 27 additions & 5 deletions railib/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class Permission(str, Enum):
"create_oauth_client",
"delete_database",
"delete_engine",
"delete_engine_wait",
"delete_model",
"disable_user",
"enable_user",
Expand All @@ -134,8 +135,13 @@ class Permission(str, Enum):
"list_oauth_clients",
"load_csv",
"update_user",
"ResourceNotFoundError",
]

class ResourceNotFoundError(Exception):
"""An error response, typically triggered by a 412 response (for update) or 404 (for get/post)"""
pass


# Context contains the state required to make rAI API calls.
class Context(rest.Context):
Expand Down Expand Up @@ -221,11 +227,15 @@ def _get_resource(ctx: Context, path: str, key=None, **kwargs) -> Dict:
url = _mkurl(ctx, path)
rsp = rest.get(ctx, url, **kwargs)
rsp = json.loads(rsp.read())

if key:
rsp = rsp[key]
if rsp and isinstance(rsp, list):
assert len(rsp) == 1

if isinstance(rsp, list):
if len(rsp) == 0:
raise ResourceNotFoundError(f"Resource not found at {url}")
Copy link
Collaborator

@torkins torkins Jul 6, 2023

Choose a reason for hiding this comment

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

is it correct to say that historically an assertion error would be thrown instead of ResourceNotFound? (If so its good, just want to make sure we minor-version bump since surface area is changing)

Copy link
Author

@antikus antikus Jul 6, 2023

Choose a reason for hiding this comment

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

Nope, the assert was there to check if for some reason we get two resources with the same name. I don't think it was useful since the name is required to be unique for any resource. But even if we do get two resources with the same name for some reason the assert wouldn't do any good for the user as the user would just get confused.

The change is backward incompatible, previously the function would return an empty list if a requested resource not found. So any method like get_engine or get_database would return an empty list when we can't find a resource with the provided name. Ideally, our rest api should handle this and give 404 but since we don't have a rest api for it I emulate this behavior by throwing ResourceNotFoundError.

Copy link
Collaborator

@torkins torkins Jul 7, 2023

Choose a reason for hiding this comment

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

got it, so we're replacing empty list with an error condition to match what a user of a rest api would expect (in this case that they get 1 resource and instead that resource doesn't exist)

return rsp[0]

return rsp


Expand Down Expand Up @@ -356,8 +366,8 @@ def poll_with_specified_overhead(
time.sleep(duration)


def is_engine_term_state(state: str) -> bool:
return state == "PROVISIONED" or ("FAILED" in state)
def is_engine_term_state(state: str, targetState: str) -> bool:
Copy link
Collaborator

@torkins torkins Jul 6, 2023

Choose a reason for hiding this comment

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

I don't quite understand the change being made here, or at least I think this function name should change to be more clear what the intent is as it no longer just returns boolean 'if engine is in a term state'. Since "PROVISIONED" is passed in lower down, whats the point here?

Copy link
Collaborator

@torkins torkins Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh I caught it... DELETED is also used below.. so is DELETED not a term state? why wouldn't DELETED be added here?

Copy link
Author

@antikus antikus Jul 7, 2023

Choose a reason for hiding this comment

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

I think to be precise and to avoid a use case where you call create_engine but in response it says DELETED but you still think it got created. This is also how it's written in our Go SDK.

return state == targetState or ("FAILED" in state)


def create_engine(ctx: Context, engine: str, size: str = "XS", **kwargs):
Expand All @@ -370,7 +380,7 @@ def create_engine(ctx: Context, engine: str, size: str = "XS", **kwargs):
def create_engine_wait(ctx: Context, engine: str, size: str = "XS", **kwargs):
create_engine(ctx, engine, size, **kwargs)
poll_with_specified_overhead(
lambda: is_engine_term_state(get_engine(ctx, engine)["state"]),
lambda: is_engine_term_state(get_engine(ctx, engine)["state"], "PROVISIONED"),
overhead_rate=0.2,
timeout=30 * 60,
)
Expand Down Expand Up @@ -416,6 +426,18 @@ def delete_engine(ctx: Context, engine: str, **kwargs) -> Dict:
return json.loads(rsp.read())


def delete_engine_wait(ctx: Context, engine: str, **kwargs) -> bool:
rsp = delete_engine(ctx, engine, **kwargs)
rsp = rsp["status"]

while not is_engine_term_state(rsp["state"], "DELETED"):
try:
rsp = get_engine(ctx, engine)
except ResourceNotFoundError:
break
time.sleep(3)


def delete_user(ctx: Context, id: str, **kwargs) -> Dict:
url = _mkurl(ctx, f"{PATH_USER}/{id}")
rsp = rest.delete(ctx, url, None, **kwargs)
Expand Down
33 changes: 33 additions & 0 deletions test/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,38 @@ def tearDown(self):
api.delete_database(ctx, dbname)


class TestEngineAPI(unittest.TestCase):
def test_get_engine(self):
testEngine = f"python-sdk-get-eng-test-{suffix}"

# ResourceNotFoundError is raised when engine does not exist
with self.assertRaises(api.ResourceNotFoundError):
api.get_engine(ctx, testEngine, headers=custom_headers)

# an engine with the name is returned when engine exists
api.create_engine_wait(ctx, testEngine, headers=custom_headers)

rsp = api.get_engine(ctx, testEngine, headers=custom_headers)
self.assertEqual(testEngine, rsp["name"])
self.assertEqual("PROVISIONED", rsp["state"])

api.delete_engine(ctx, testEngine, headers=custom_headers)

def test_delete_engine(self):
testEngine = f"python-sdk-del-eng-test-{suffix}"

rsp = api.create_engine_wait(ctx, testEngine, headers=custom_headers)
self.assertEqual("PROVISIONED", rsp["state"])
rsp = api.delete_engine(ctx, testEngine, headers=custom_headers)
self.assertEqual("DELETING", rsp["status"]["state"])

def test_delete_engine_wait(self):
testEngine = f"python-sdk-del-eng-w-test-{suffix}"

rsp = api.create_engine_wait(ctx, testEngine, headers=custom_headers)
res = api.delete_engine_wait(ctx, testEngine, headers=custom_headers)
self.assertEqual(None, res)


if __name__ == '__main__':
unittest.main()