-
Notifications
You must be signed in to change notification settings - Fork 77
Update start workflows container script for unified storage feature to create workflows sub dir #674
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
Update start workflows container script for unified storage feature to create workflows sub dir #674
Conversation
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.
If these changes get built into a patch SMD and deployed to existing spaces (without storage), then will this break existing functionality?
template/v2/dirs/etc/sagemaker-ui/workflows/start-workflows-container.sh
Outdated
Show resolved
Hide resolved
no it shouldn't break, I've tested it in an existing Gamma space without storage too to make sure it's backwards compatible |
DZ_PROJECT_S3PATH=$(jq -r '.AdditionalMetadata.ProjectS3Path' < $RESOURCE_METADATA_FILE) | ||
|
||
# Helper to check if the project is using Git storage | ||
is_s3_storage() { |
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.
Can we reuse this definition from here - #668 as a param?
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.
Can you refactor to have these common functions in the post launch script and just use these as params within the workflows container?
template/v2/dirs/etc/sagemaker-ui/workflows/start-workflows-container.sh
Outdated
Show resolved
Hide resolved
…tart workflows local runner script
Description
For projects with unified storage(s3), there will be a project shared dir
/home/sagemaker-user/shared-files/
to replace/home/sagemaker-user/src/
, this change is mainly to adapt to this new use case: if it's using git, everything is kept the same; if it's using unified storage, then we're creating/home/sagemaker-user/shared-files/workflows/
and all the sub-directories under/workflows
Type of Change
Release Information
Does this change need to be included in patch version releases? By default, any pull requests will only be added to the next SMD image minor version release once they are merged in template folder. Only critical bug fix or security update should be applied to new patch versions of existed image minor versions.
If yes, please explain why:
[Explain the criticality of this change and why it should be included in patch releases]
How Has This Been Tested?
[Describe the tests you ran]
Checklist:
Test Screenshots (if applicable):
tested in gamma project with the new start container script, local runner was able to start and verified workflows subfolder was created under the new "shared-files" project directory:
Tested the following scenarios:
bash /etc/sagemaker-ui/workflows/start-workflows-container.sh
(backwards compatibility)bash /etc/sagemaker-ui/workflows/start-workflows-container.sh "$is_s3_flag”
(with is_s3_flag=1, Git project)bash /etc/sagemaker-ui/workflows/start-workflows-container.sh "$is_s3_flag”
(is_s3_flag=0, shared storage project: Project is using S3 storage, project directory set to: /home/sagemaker-user/shared-files)Related Issues
[Link any related issues here]
Additional Notes
[Any additional information that might be helpful for reviewers]