Skip to content

Commit ac33d67

Browse files
authored
fix(sagemaker): GetStatus error when refreshing large number of spaces and fix deeplink reconnect (aws#2161)
## Problem - When there are a large number of SageMaker space nodes, refreshing the Explorer tree can take time. During this window, the underlying node objects may be temporarily undefined, causing actions like "Connect" or "Stop" to fail. This happens because all space nodes were previously being refreshed whenever an action was taken on any one node, or when the Explorer tree was refreshed. - Deeplink reconnect can fail if the user is not logged in with the same credentials originally used to describe the space. This is because the Toolkit currently makes a `DescribeSpace` call using local credentials. Ideally, Studio UI should pass the `appType` directly in the deeplink, avoiding the need for Toolkit to make this call. However, since changes to Studio UI can’t be deployed before the NY Summit, this update will be handled in a future MFE release. ## Solution - Updated the logic to **refresh only the specific space node** being acted on, instead of refreshing all nodes. This avoids unnecessary delays and reduces the likelihood of undefined node states during actions. - Added a **warning message** when the full space list is being refreshed. If a user tries to interact with a space during this time, they will see a message indicating that space information is still loading and to try again shortly. - Temporarily **hardcoded the `appType` to `JupyterLab`** in the reconnect URI for all app types. Reconnection will still work for both Code Editor and JupyterLab, although the URL path will always show `/jupyterlab`. This is a temporary workaround until Studio UI can send the correct `appType`. - Added **telemetry for deeplink connect** --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 3c1c282 commit ac33d67

File tree

7 files changed

+55
-32
lines changed

7 files changed

+55
-32
lines changed

packages/core/src/awsService/sagemaker/activation.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import * as path from 'path'
7+
import * as vscode from 'vscode'
78
import { Commands } from '../../shared/vscode/commands2'
89
import { SagemakerSpaceNode } from './explorer/sagemakerSpaceNode'
910
import { SagemakerParentNode } from './explorer/sagemakerParentNode'
@@ -20,6 +21,9 @@ export async function activate(ctx: ExtContext): Promise<void> {
2021
ctx.extensionContext.subscriptions.push(
2122
uriHandlers.register(ctx),
2223
Commands.register('aws.sagemaker.openRemoteConnection', async (node: SagemakerSpaceNode) => {
24+
if (!validateNode(node)) {
25+
return
26+
}
2327
await telemetry.sagemaker_openRemoteConnection.run(async () => {
2428
await openRemoteConnect(node, ctx.extensionContext)
2529
})
@@ -32,6 +36,9 @@ export async function activate(ctx: ExtContext): Promise<void> {
3236
}),
3337

3438
Commands.register('aws.sagemaker.stopSpace', async (node: SagemakerSpaceNode) => {
39+
if (!validateNode(node)) {
40+
return
41+
}
3542
await telemetry.sagemaker_stopSpace.run(async () => {
3643
await stopSpace(node, ctx.extensionContext)
3744
})
@@ -62,3 +69,14 @@ export async function activate(ctx: ExtContext): Promise<void> {
6269
})
6370
}
6471
}
72+
73+
/**
74+
* Checks if a node is undefined and shows a warning message if so.
75+
*/
76+
function validateNode(node: unknown): boolean {
77+
if (!node) {
78+
void vscode.window.showWarningMessage('Space information is being refreshed. Please try again shortly.')
79+
return false
80+
}
81+
return true
82+
}

packages/core/src/awsService/sagemaker/credentialMapping.ts

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ import { DevSettings } from '../../shared/settings'
1212
import { Auth } from '../../auth/auth'
1313
import { SpaceMappings, SsmConnectionInfo } from './types'
1414
import { getLogger } from '../../shared/logger/logger'
15-
import { SagemakerClient } from '../../shared/clients/sagemaker'
16-
import { AppType } from '@amzn/sagemaker-client'
1715
import { parseArn } from './detached-server/utils'
1816

1917
const mappingFileName = '.sagemaker-space-profiles'
@@ -83,25 +81,13 @@ export async function persistSSMConnection(
8381
wsUrl?: string,
8482
token?: string
8583
): Promise<void> {
86-
const { region, spaceName } = parseArn(appArn)
84+
const { region } = parseArn(appArn)
8785
const endpoint = DevSettings.instance.get('endpoints', {})['sagemaker'] ?? ''
88-
const client = new SagemakerClient(region)
8986

90-
const spaceDetails = await client.describeSpace({
91-
DomainId: domain,
92-
SpaceName: spaceName,
93-
})
94-
95-
let appSubDomain: string
96-
if (spaceDetails.SpaceSettings?.AppType === AppType.JupyterLab) {
97-
appSubDomain = 'jupyterlab'
98-
} else if (spaceDetails.SpaceSettings?.AppType === AppType.CodeEditor) {
99-
appSubDomain = 'code-editor'
100-
} else {
101-
throw new ToolkitError(
102-
`Unsupported or missing app type for space. Expected JupyterLab or CodeEditor, got: ${spaceDetails.SpaceSettings?.AppType ?? 'undefined'}`
103-
)
104-
}
87+
// TODO: Hardcoded to 'jupyterlab' due to a bug in Studio that only supports refreshing
88+
// the token for both CodeEditor and JupyterLab Apps in the jupyterlab subdomain.
89+
// This will be fixed shortly after NYSummit launch to support refresh URL in CodeEditor subdomain.
90+
const appSubDomain = 'jupyterlab'
10591

10692
let envSubdomain: string
10793

packages/core/src/awsService/sagemaker/explorer/sagemakerSpaceNode.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,15 +162,17 @@ export class SagemakerSpaceNode extends AWSTreeNodeBase implements AWSResourceNo
162162

163163
public async refreshNode(): Promise<void> {
164164
await this.updateSpaceAppStatus()
165-
await tryRefreshNode(this)
165+
await vscode.commands.executeCommand('aws.refreshAwsExplorerNode', this)
166166
}
167167
}
168168

169169
export async function tryRefreshNode(node?: SagemakerSpaceNode) {
170170
if (node) {
171-
const n = node instanceof SagemakerSpaceNode ? node.parent : node
172171
try {
173-
await n.refreshNode()
172+
// For SageMaker spaces, refresh just the individual space node to avoid expensive
173+
// operation of refreshing all spaces in the domain
174+
await node.updateSpaceAppStatus()
175+
await vscode.commands.executeCommand('aws.refreshAwsExplorerNode', node)
174176
} catch (e) {
175177
getLogger().error('refreshNode failed: %s', (e as Error).message)
176178
}

packages/core/src/awsService/sagemaker/model.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ export async function startLocalServer(ctx: vscode.ExtensionContext) {
121121
const errLog = path.join(storagePath, 'sagemaker-local-server.err.log')
122122
const infoFilePath = path.join(storagePath, 'sagemaker-local-server-info.json')
123123

124-
logger.info(`local server logs at ${storagePath}/sagemaker-local-server.*.log`)
124+
logger.info(`sagemaker-local-server.*.log at ${storagePath}`)
125125

126126
const customEndpoint = DevSettings.instance.get('endpoints', {})['sagemaker']
127127

packages/core/src/awsService/sagemaker/uriHandlers.ts

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,20 @@ import * as vscode from 'vscode'
77
import { SearchParams } from '../../shared/vscode/uriHandler'
88
import { deeplinkConnect } from './commands'
99
import { ExtContext } from '../../shared/extensions'
10+
import { telemetry } from '../../shared/telemetry/telemetry'
1011

1112
export function register(ctx: ExtContext) {
1213
async function connectHandler(params: ReturnType<typeof parseConnectParams>) {
13-
await deeplinkConnect(
14-
ctx,
15-
params.connection_identifier,
16-
params.session,
17-
`${params.ws_url}&cell-number=${params['cell-number']}`,
18-
params.token,
19-
params.domain
20-
)
14+
await telemetry.sagemaker_deeplinkConnect.run(async () => {
15+
await deeplinkConnect(
16+
ctx,
17+
params.connection_identifier,
18+
params.session,
19+
`${params.ws_url}&cell-number=${params['cell-number']}`,
20+
params.token,
21+
params.domain
22+
)
23+
})
2124
}
2225

2326
return vscode.Disposable.from(ctx.uriHandler.onPath('/connect/sagemaker', connectHandler, parseConnectParams))

packages/core/src/shared/telemetry/vscodeTelemetry.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,15 @@
268268
}
269269
]
270270
},
271+
{
272+
"name": "sagemaker_deeplinkConnect",
273+
"description": "Connect to SageMake Space via a deeplink",
274+
"metadata": [
275+
{
276+
"type": "result"
277+
}
278+
]
279+
},
271280
{
272281
"name": "amazonq_didSelectProfile",
273282
"description": "Emitted after the user's Q Profile has been set, whether the user was prompted with a dialog, or a profile was automatically assigned after signing in.",

packages/core/src/test/awsService/sagemaker/credentialMapping.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,12 @@ describe('credentialMapping', () => {
181181
)
182182
})
183183

184-
it('throws error when app type is unsupported', async () => {
184+
// TODO: Skipped due to hardcoded appSubDomain. Currently hardcoded to 'jupyterlab' due to
185+
// a bug in Studio that only supports refreshing the token for both CodeEditor and JupyterLab
186+
// Apps in the jupyterlab subdomain. This will be fixed shortly after NYSummit launch to
187+
// support refresh URL in CodeEditor subdomain. Additionally, appType will be determined by
188+
// the deeplink URI rather than the describeSpace call from the toolkit.
189+
it.skip('throws error when app type is unsupported', async () => {
185190
sandbox.stub(DevSettings.instance, 'get').returns({})
186191
sandbox.stub(fs, 'existsFile').resolves(false)
187192

0 commit comments

Comments
 (0)