-
Notifications
You must be signed in to change notification settings - Fork 39
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
Improved model deployment progress bar #144
Conversation
poll_interval: int = DEFAULT_POLL_INTERVAL, | ||
): | ||
"""_wait_for_progress_completion blocks until progress is completed. | ||
def _wait_for_work_request( |
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 the ticket it says that progress bar should be implemented as part of a Mixin class, but the implementation is different. Shouldn't we have some solution that can be reused for the different services the we do support?
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, the method has been moved to class OCIWorkRequestMixin
for general use.
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.
Even though I think there is still room for improvement. This update simplified the existing logic and provided a basic reusable method. We can keep improving it when we have additional use cases.
@@ -29,6 +30,10 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
LIFECYCLE_STOP_STATE = ("SUCCEEDED", "FAILED", "CANCELED", "DELETED") | |||
WORK_REQUEST_STOP_STATE = ("SUCCEEDED", "FAILED", "CANCELED") |
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.
This might depend on the service.
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.
These are the WorkRequest
terminal states and do the services have different values?
def wait_for_progress( | ||
self, | ||
work_request_id: str, | ||
num_steps: int = DEFAULT_WORKFLOW_STEPS, |
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 does not seem like there is an easy way to know the number of workflow steps for different workflow. Having a default here could be confusing for the developers. I think a better behavior is, while the number of steps is not specified, it will continue waiting until work_request.status reach WORK_REQUEST_STOP_STATE. Even though WORK_REQUEST_STOP_STATE still depends on the workflow, most, if not all workflow has SUCCEEDED and FAILED states.
Improved model deployment progress bar
OCIWorkRequestMixin
class so that it can be used by other services.Notebook