-
Notifications
You must be signed in to change notification settings - Fork 33
Add SageMaker-UI poststartup endpoint patch #111
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
Conversation
**Description** - Post-startup script stored in SMD (SageMaker Distribution) enables the required customizations for SMUS applications. - This functionality is currently operational for JupyterLab apps. - We are extending this capability to CodeEditor apps. **Testing Done** - Tested building the local image for these changes and tested on personal LL stack using BYOI - Verified script execution logs file generation at /var/log/apps after app launch
| * Handles API requests to run the post-startup script in SMD. | ||
| */ | ||
| private async _handlePostStartupScriptInvocation(req: http.IncomingMessage, res: http.ServerResponse): Promise<void> { | ||
| const postStartupScripPath = '/etc/sagemaker-ui/sagemaker_ui_post_startup.sh' |
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.
typo
| // Adding 0o755 to make script file executable | ||
| fs.chmodSync(postStartupScripPath, 0o755); | ||
|
|
||
| const subprocess = spawn('bash', [`${postStartupScripPath}`], { cwd: '/' }); |
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.
subprocess errors will not be caught in catch block. see https://stackoverflow.com/questions/48113929/why-does-not-a-try-catch-block-catch-a-child-process-spawn-exception-due-to-inva/48114608
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 happens only in case of invalid executable path. In our case the script path should always be correct since we are providing it, even if it is incorrect we perform fileExist check which throws 500 error.
- In case of invalid script execution command, the error gets caught and again the server throws 500 error.
- I verified this by running the server locally.
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.
how do we handle errors thrown by the script itself?
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 api just triggers the script execution. If there are errors while executing the script, they get logged in the log file.
Add SageMaker-UI poststartup endpoint patch
Description
Testing Done
Issue #, if available:
Description of changes:
Adding
/api/poststartupendpoint for the SageMaker Unified Studio user case that runs the post startup script.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.