-
Notifications
You must be signed in to change notification settings - Fork 431
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
Enhancing the orchestrator UX for major cloud providers #3005
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Please don't merge them. Some of them are overlapping, yes, but they generally are used for different purposes.
This would be even worse, as running |
|
||
# List the Studio domains and get the Studio Domain ID | ||
# TODO: Solve this with the config | ||
domains_response = session.sagemaker_client.list_domains() |
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's the issue with using any domain? That it won't work? Or lead them to a broken link?
Somehow not a huge fan of having a config attribute just to make a link work
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 would work as long as they have at least one domain. In this case, we take the first domain and then use it to create the link. I am not sure how people are scoping domains though, I lack the Sagemaker UX knowledge here a bit. I thought a config attribute would help them to specify which domain to use but I am also OK with leaving it like this.
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 guess right now it would just fail if they have 0 domains, if that's a possibility?
Don't have a real opinion here though, do whatever you think makes sense, but if you don't know what makes sense, maybe leave it out for now and see if people complain?
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 mean it would not "fail". In that case, we won't be creating any URLs as it is impossible, that's it. The rest of the code will execute as intended. Leaving it as is if @stefannica is ok with it as well.
…r.py Co-authored-by: Michael Schuster <[email protected]>
…r.py Co-authored-by: Michael Schuster <[email protected]>
Co-authored-by: Michael Schuster <[email protected]>
@schustmi as a response to your general comment: You are right, I haven't implemented the change in the cleanup logic just yet due to time limitations. I am not sure if I can squeeze it right now but in the worst case, I will handle it in a separate PR. |
Describe changes
This PR aims to solve two problems at the same time:
Implementation details
For task number 1 (URLs)
run_utils.deploy_pipeline
andStack.deploy_pipeline
calls to pass theplaceholder_run
forward.BaseOrchestrator.run
to accept the placeholder run. During this call, we fetch everything thatprepare_or_run_pipeline
yield
s and we publish it as metadata.azureml
,sagemaker
andvertex
orchestrators to use their respective clients (and createdJob
objects) toyield
theirorchestrator_run_id
s,orchestrator_log_url
andorchestrator_url
s preemptively if possible. This helps us to display the URLs, even before the first step starts.For task number 2 (Pipeline Status Refresh)
sagemaker
,azureml
andvertex
.base.Dockerfile
andzenml-server-dev.Dockerfile
to install these new extras as well.fetch_status
method to theBaseOrchestrator
implementation, which on default throws aNotImplementedError
.refresh_status
to thePipelineRunResponse
model which makes use of the aforementionedfetch_status
method.fetch_status
implementation forvertex
,sagemaker
, andazureml
orchestrators.runs/<RUN_ID>/refresh
which refreshes the status of a pipeline run if possible.Get Run
endpoint with arefresh_status
flag that will return a response with a refreshed status if possible.zenml pipeline runs refresh <RUN_ID>
) to refresh the status of a pipeline manually.Docs Updates
refresh
CLI command to the CLI docsTODOs
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes