-
Notifications
You must be signed in to change notification settings - Fork 747
fix(amazonq): fix for amazon q app initialization failure on sagemaker #7548
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
|
| (Experiments.instance.get('amazonqLSP', true) || Auth.instance.isInternalAmazonUser()) && | ||
| (!isAmazonLinux2() || hasGlibcPatch()) | ||
| ) { | ||
| (isSageMaker() || !isAmazonLinux2() || (await hasGlibcPatch())) |
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 appears that the isAmazonLinux2 check is incorrectly flagging ubuntu containers under AL2 hosts. i do not think we need to special case sagemaker for now
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 is the exact setup for SageMaker AL2 instances?
| "@aws-sdk/smithy-client": "<3.731.0", | ||
| "@aws-sdk/util-arn-parser": "<3.731.0", | ||
| "@aws/mynah-ui": "^4.35.4", | ||
| "@aws/mynah-ui": "^4.35.6", |
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.
mynah update should not be in this focused change
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.
sorry..previous commit sneaked in...let me correct and raise a new one
|
What is the root cause of this issue? If SageMaker does not have such env var, why is it going to the GLIBC patch code path |
|
like richard said...that AL2 check |
| * @returns true if the current system is SageMaker(SMAI or SMUS) | ||
| */ | ||
| export function isSageMaker(appName: 'SMAI' | 'SMUS' = 'SMAI'): boolean { | ||
| // Check for SageMaker-specific environment variables first |
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.
FYI, the check for Unified Studio i.e process.env.SERVICE_NAME === sageMakerUnifiedStudio, is already handled in isSageMakerUnifiedStudio()
And with this logic, it will bypass the check for Sagemaker AI or Unified Studio cases right? Can we extract these env variable checks omitting this process.env.SERVICE_NAME === sageMakerUnifiedStudiointo a boolean and use it in case statements for better readability?
Problem
GlibC path execution failing for sagemaker instances due to changes made to support AL2.
Solution
feature/xbranches will not be squash-merged at release time.