-
Notifications
You must be signed in to change notification settings - Fork 86
fix: extend SageMaker env detection to support both SMUS and SMAI #2598
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
ef40411 to
adb3e31
Compare
| ? lspParams?.initializationOptions?.aws?.clientInfo?.name | ||
| : lspParams?.clientInfo?.name | ||
| return lspParams?.initializationOptions?.aws?.clientInfo?.name || lspParams?.clientInfo?.name | ||
| } |
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.
some notes & context (@aws-srijach please add to the description):
- using
lspParams?.initializationOptions?.aws?.clientInfo?.nameonly for unified studio was done for code editor support, but this breaks SMUS jupyter lab support, which useslspParams?.clientInfo?.name. hence the move to|| - we (sagemaker AI studio, seperate from unified studio) need to use
lspParams?.initializationOptions?.aws?.clientInfo?.nameas well. I would strongly prefer to remove the environment variable check as (1) it does not scale if and when more folks want to use this (2) we do not have an easy env variable flag to add here (3) this environment variable flag provides no protection, why would a non-SMUS service set this configuration option if it's a no-op?
let me know if folks have concerns
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.
update - I see nwo that initializationOptions?.aws?.clientInfo?.name is used for multiple purposes. looking into how we can gate this since other clients will set 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.
we decided to gate this so all SM setups will use lspParams?.initializationOptions?.aws?.clientInfo?.name || lspParams?.clientInfo?.name. this works as:
- CE will remain the same, pulling from
lspParams?.initializationOptions?.aws?.clientInfo?.name - JL will still use
lspParams?.clientInfo?.name, aslspParams?.initializationOptions?.aws?.clientInfo?.nameis not set. we have confirmed this in SMUS and SMAI, we are testing the final set of changes together currently.
7d615f1 to
30e59d8
Compare
30e59d8 to
48f05ba
Compare
aakashmandavilli96
left a comment
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.
LGTM
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2598 +/- ##
==========================================
+ Coverage 60.60% 60.65% +0.04%
==========================================
Files 278 278
Lines 65227 65238 +11
Branches 4154 4161 +7
==========================================
+ Hits 39533 39568 +35
+ Misses 25610 25586 -24
Partials 84 84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Problem
The
getClientNamefunction was checking for a specific environment variable (SERVICE_NAME === SAGEMAKER_UNIFIED_STUDIO_SERVICE) to determine whether to check bothinitializationOptions.aws.clientInfo.nameandclientInfo.name. This limited the logic to only SMUS environments.Solution
isSagemakerEnv()function that detects both SMUS and SMAI using environment variablesgetClientName()to only check when in SageMaker environmentsclientInfo.nameNotes
lspParams?.initializationOptions?.aws?.clientInfo?.nameonly for unified studio was done for code editor support, but this breaks SMUS jupyter lab support, which useslspParams?.clientInfo?.namehence the move to||Testing
Tested in both SMUS and SMAI environments for JupyterLab and CodeEditor