Skip to content

Commit df9ef5d

Browse files
feat(smus): UX improvements for connecting running spaces from toolkit (aws#8261)
## Problem - Connect button only appeared for spaces with remote access enabled. When users landed on the toolkit and saw running spaces without remote access, there was no connect button visible. This created confusion because: -- Users expected to be able to connect to any running space -- No visual indication that connection was possible -- Users didn't understand why some spaces were "connectable" and others weren't ## Solution - Always show connect button for all spaces (running and stopped, regardless of remote access status). When users click connect on a space without remote access, show clear dialog explaining what needs to happen and get their consent upfront. - There is no breaking change. - Tested for all use-cases SM-AI and SMUS ## Current User Exp --- https://github.com/user-attachments/assets/9c5c8634-42df-4cd1-988b-12cc8008d69e - 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 ef992ae commit df9ef5d

File tree

9 files changed

+692
-90
lines changed

9 files changed

+692
-90
lines changed

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

Lines changed: 198 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,19 @@ import _ from 'lodash'
1616
import { prepareDevEnvConnection, tryRemoteConnection } from './model'
1717
import { ExtContext } from '../../shared/extensions'
1818
import { SagemakerClient } from '../../shared/clients/sagemaker'
19+
import { AccessDeniedException } from '@amzn/sagemaker-client'
1920
import { ToolkitError } from '../../shared/errors'
2021
import { showConfirmationMessage } from '../../shared/utilities/messages'
2122
import { RemoteSessionError } from '../../shared/remoteSession'
22-
import { ConnectFromRemoteWorkspaceMessage, InstanceTypeError } from './constants'
23+
import {
24+
ConnectFromRemoteWorkspaceMessage,
25+
InstanceTypeError,
26+
InstanceTypeInsufficientMemory,
27+
InstanceTypeInsufficientMemoryMessage,
28+
RemoteAccess,
29+
RemoteAccessRequiredMessage,
30+
SpaceStatus,
31+
} from './constants'
2332
import { SagemakerUnifiedStudioSpaceNode } from '../../sagemakerunifiedstudio/explorer/nodes/sageMakerUnifiedStudioSpaceNode'
2433

2534
const localize = nls.loadMessageBundle()
@@ -136,10 +145,10 @@ export async function stopSpace(
136145
sageMakerClient?: SagemakerClient
137146
) {
138147
await tryRefreshNode(node)
139-
if (node.getStatus() === 'Stopped' || node.getStatus() === 'Stopping') {
148+
if (node.getStatus() === SpaceStatus.STOPPED || node.getStatus() === SpaceStatus.STOPPING) {
140149
void vscode.window.showWarningMessage(`Space ${node.spaceApp.SpaceName} is already in Stopped/Stopping state.`)
141150
return
142-
} else if (node.getStatus() === 'Starting') {
151+
} else if (node.getStatus() === SpaceStatus.STARTING) {
143152
void vscode.window.showWarningMessage(
144153
`Space ${node.spaceApp.SpaceName} is in Starting state. Wait until it is Running to attempt stop again.`
145154
)
@@ -167,7 +176,7 @@ export async function stopSpace(
167176
})
168177
} catch (err) {
169178
const error = err as Error
170-
if (error.name === 'AccessDeniedException') {
179+
if (error instanceof AccessDeniedException) {
171180
throw new ToolkitError('You do not have permission to stop spaces. Please contact your administrator', {
172181
cause: error,
173182
code: error.name,
@@ -195,56 +204,205 @@ export async function openRemoteConnect(
195204
const spaceName = node.spaceApp.SpaceName!
196205
await tryRefreshNode(node)
197206

198-
// for Stopped SM spaces - check instance type before showing progress
199-
if (node.getStatus() === 'Stopped') {
200-
// In case of SMUS, we pass in a SM Client and for SM AI, it creates a new SM Client.
201-
const client = sageMakerClient ? sageMakerClient : new SagemakerClient(node.regionCode)
202-
203-
try {
204-
await client.startSpace(spaceName, node.spaceApp.DomainId!)
205-
await tryRefreshNode(node)
206-
const appType = node.spaceApp.SpaceSettingsSummary?.AppType
207-
if (!appType) {
208-
throw new ToolkitError('AppType is undefined for the selected space. Cannot start remote connection.', {
209-
code: 'undefinedAppType',
210-
})
207+
const remoteAccess = node.spaceApp.SpaceSettingsSummary?.RemoteAccess
208+
const nodeStatus = node.getStatus()
209+
210+
// Route to appropriate handler based on space state
211+
if (nodeStatus === SpaceStatus.RUNNING && remoteAccess !== RemoteAccess.ENABLED) {
212+
return handleRunningSpaceWithDisabledAccess(node, ctx, spaceName, sageMakerClient)
213+
} else if (nodeStatus === SpaceStatus.STOPPED) {
214+
return handleStoppedSpace(node, ctx, spaceName, sageMakerClient)
215+
} else if (nodeStatus === SpaceStatus.RUNNING) {
216+
return handleRunningSpaceWithEnabledAccess(node, ctx, spaceName)
217+
}
218+
}
219+
220+
/**
221+
* Checks if an instance type upgrade will be needed for remote access
222+
*/
223+
export async function checkInstanceTypeUpgradeNeeded(
224+
node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
225+
sageMakerClient?: SagemakerClient
226+
): Promise<{ upgradeNeeded: boolean; currentType?: string; recommendedType?: string }> {
227+
const client = sageMakerClient || new SagemakerClient(node.regionCode)
228+
229+
try {
230+
const spaceDetails = await client.describeSpace({
231+
DomainId: node.spaceApp.DomainId!,
232+
SpaceName: node.spaceApp.SpaceName!,
233+
})
234+
235+
const appType = spaceDetails.SpaceSettings!.AppType!
236+
237+
// Get current instance type
238+
const currentResourceSpec =
239+
appType === 'JupyterLab'
240+
? spaceDetails.SpaceSettings!.JupyterLabAppSettings?.DefaultResourceSpec
241+
: spaceDetails.SpaceSettings!.CodeEditorAppSettings?.DefaultResourceSpec
242+
243+
const currentInstanceType = currentResourceSpec?.InstanceType
244+
245+
// Check if upgrade is needed
246+
if (currentInstanceType && currentInstanceType in InstanceTypeInsufficientMemory) {
247+
// Current type has insufficient memory
248+
return {
249+
upgradeNeeded: true,
250+
currentType: currentInstanceType,
251+
recommendedType: InstanceTypeInsufficientMemory[currentInstanceType],
211252
}
253+
}
254+
255+
return { upgradeNeeded: false, currentType: currentInstanceType }
256+
} catch (err) {
257+
const error = err as Error
258+
if (error instanceof AccessDeniedException) {
259+
throw new ToolkitError('You do not have permission to describe spaces. Please contact your administrator', {
260+
cause: error,
261+
code: error.name,
262+
})
263+
}
264+
throw err
265+
}
266+
}
267+
268+
/**
269+
* Handles connecting to a running space with disabled remote access
270+
* Requires stopping the space, enabling remote access, and restarting
271+
*/
272+
async function handleRunningSpaceWithDisabledAccess(
273+
node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
274+
ctx: vscode.ExtensionContext,
275+
spaceName: string,
276+
sageMakerClient?: SagemakerClient
277+
) {
278+
// Check if instance type upgrade will be needed
279+
const instanceTypeInfo = await checkInstanceTypeUpgradeNeeded(node, sageMakerClient)
280+
281+
let prompt: string
282+
if (instanceTypeInfo.upgradeNeeded) {
283+
prompt = InstanceTypeInsufficientMemoryMessage(
284+
spaceName,
285+
instanceTypeInfo.currentType!,
286+
instanceTypeInfo.recommendedType!
287+
)
288+
} else {
289+
// Only remote access needs to be enabled
290+
prompt = RemoteAccessRequiredMessage
291+
}
292+
293+
const confirmed = await showConfirmationMessage({
294+
prompt,
295+
confirm: 'Restart and Connect',
296+
cancel: 'Cancel',
297+
type: 'warning',
298+
})
299+
300+
if (!confirmed) {
301+
return
302+
}
303+
304+
// Enable remote access and connect
305+
const client = sageMakerClient || new SagemakerClient(node.regionCode)
306+
307+
return await vscode.window.withProgress(
308+
{
309+
location: vscode.ProgressLocation.Notification,
310+
cancellable: false,
311+
title: `Connecting to ${spaceName}`,
312+
},
313+
async (progress) => {
314+
try {
315+
// Show initial progress message
316+
progress.report({ message: 'Stopping the space' })
317+
318+
// Stop the running space
319+
await client.deleteApp({
320+
DomainId: node.spaceApp.DomainId!,
321+
SpaceName: spaceName,
322+
AppType: node.spaceApp.App!.AppType!,
323+
AppName: node.spaceApp.App?.AppName,
324+
})
212325

213-
// Only start showing progress after instance type validation
214-
return await vscode.window.withProgress(
215-
{
216-
location: vscode.ProgressLocation.Notification,
217-
cancellable: false,
218-
title: `Connecting to ${spaceName}`,
219-
},
220-
async (progress) => {
221-
progress.report({ message: 'Starting the space.' })
222-
await client.waitForAppInService(node.spaceApp.DomainId!, spaceName, appType)
223-
await tryRemoteConnection(node, ctx, progress)
326+
// Update progress message
327+
progress.report({ message: 'Starting the space' })
328+
329+
// Start the space with remote access enabled (skip prompts since user already consented)
330+
await client.startSpace(spaceName, node.spaceApp.DomainId!, true)
331+
await tryRefreshNode(node)
332+
await client.waitForAppInService(node.spaceApp.DomainId!, spaceName, node.spaceApp.App!.AppType!)
333+
await tryRemoteConnection(node, ctx, progress)
334+
} catch (err: any) {
335+
// Handle user declining instance type upgrade
336+
if (err.code === InstanceTypeError) {
337+
return
224338
}
225-
)
226-
} catch (err: any) {
227-
// Ignore InstanceTypeError since it means the user decided not to use an instanceType with more memory
228-
// just return without showing progress
229-
if (err.code === InstanceTypeError) {
230-
return
339+
throw new ToolkitError(`Remote connection failed: ${err.message}`, {
340+
cause: err,
341+
code: err.code,
342+
})
231343
}
232-
throw new ToolkitError(`Remote connection failed: ${(err as Error).message}`, {
233-
cause: err as Error,
234-
code: err.code,
235-
})
236344
}
237-
} else if (node.getStatus() === 'Running') {
238-
// For running spaces, show progress
345+
)
346+
}
347+
348+
/**
349+
* Handles connecting to a stopped space
350+
* Starts the space and connects (remote access enabled automatically if needed)
351+
*/
352+
async function handleStoppedSpace(
353+
node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
354+
ctx: vscode.ExtensionContext,
355+
spaceName: string,
356+
sageMakerClient?: SagemakerClient
357+
) {
358+
const client = sageMakerClient || new SagemakerClient(node.regionCode)
359+
360+
try {
361+
await client.startSpace(spaceName, node.spaceApp.DomainId!)
362+
await tryRefreshNode(node)
363+
239364
return await vscode.window.withProgress(
240365
{
241366
location: vscode.ProgressLocation.Notification,
242367
cancellable: false,
243368
title: `Connecting to ${spaceName}`,
244369
},
245370
async (progress) => {
371+
progress.report({ message: 'Starting the space' })
372+
await client.waitForAppInService(node.spaceApp.DomainId!, spaceName, node.spaceApp.App!.AppType!)
246373
await tryRemoteConnection(node, ctx, progress)
247374
}
248375
)
376+
} catch (err: any) {
377+
// Handle user declining instance type upgrade
378+
if (err.code === InstanceTypeError) {
379+
return
380+
}
381+
throw new ToolkitError(`Remote connection failed: ${(err as Error).message}`, {
382+
cause: err as Error,
383+
code: err.code,
384+
})
249385
}
250386
}
387+
388+
/**
389+
* Handles connecting to a running space with enabled remote access
390+
* Direct connection without any space modifications
391+
*/
392+
async function handleRunningSpaceWithEnabledAccess(
393+
node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
394+
ctx: vscode.ExtensionContext,
395+
spaceName: string,
396+
sageMakerClient?: SagemakerClient
397+
) {
398+
return await vscode.window.withProgress(
399+
{
400+
location: vscode.ProgressLocation.Notification,
401+
cancellable: false,
402+
title: `Connecting to ${spaceName}`,
403+
},
404+
async (progress) => {
405+
await tryRemoteConnection(node, ctx, progress)
406+
}
407+
)
408+
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,19 @@ export const InstanceTypeInsufficientMemory: Record<string, string> = {
1818
'ml.c5.large': 'ml.c5.xlarge',
1919
}
2020

21+
// Remote access constants
22+
export const RemoteAccess = {
23+
ENABLED: 'ENABLED',
24+
DISABLED: 'DISABLED',
25+
} as const
26+
27+
export const SpaceStatus = {
28+
RUNNING: 'Running',
29+
STOPPED: 'Stopped',
30+
STARTING: 'Starting',
31+
STOPPING: 'Stopping',
32+
} as const
33+
2134
export const InstanceTypeInsufficientMemoryMessage = (
2235
spaceName: string,
2336
chosenInstanceType: string,
@@ -29,3 +42,6 @@ export const InstanceTypeInsufficientMemoryMessage = (
2942
export const InstanceTypeNotSelectedMessage = (spaceName: string) => {
3043
return `No instanceType specified for [${spaceName}]. ${InstanceTypeMinimum} is the default instance type, which meets minimum 8 GiB memory requirements for remote access. Continuing will start your space with instanceType [${InstanceTypeMinimum}] and remotely connect.`
3144
}
45+
46+
export const RemoteAccessRequiredMessage =
47+
'This space requires remote access to be enabled.\nWould you like to restart the space and connect?\nAny unsaved work will be lost.'

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

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { getIcon, IconPath } from '../../shared/icons'
1111
import { generateSpaceStatus, updateIdleFile, startMonitoringTerminalActivity, ActivityCheckInterval } from './utils'
1212
import { UserActivity } from '../../shared/extensionUtilities'
1313
import { getLogger } from '../../shared/logger/logger'
14+
import { SpaceStatus, RemoteAccess } from './constants'
1415

1516
export class SagemakerSpace {
1617
public label: string = ''
@@ -53,7 +54,7 @@ export class SagemakerSpace {
5354
}
5455

5556
public isPending(): boolean {
56-
return this.getStatus() !== 'Running' && this.getStatus() !== 'Stopped'
57+
return this.getStatus() !== SpaceStatus.RUNNING && this.getStatus() !== SpaceStatus.STOPPED
5758
}
5859

5960
public getStatus(): string {
@@ -148,8 +149,18 @@ export class SagemakerSpace {
148149
const domainId = this.spaceApp?.DomainId ?? '-'
149150
const owner = this.spaceApp?.OwnershipSettingsSummary?.OwnerUserProfileName || '-'
150151
const instanceType = this.spaceApp?.App?.ResourceSpec?.InstanceType ?? '-'
152+
const remoteAccess = this.spaceApp?.SpaceSettingsSummary?.RemoteAccess
153+
154+
let baseTooltip = ''
151155
if (this.isSMUSSpace) {
152-
return `**Space:** ${spaceName} \n\n**Application:** ${appType} \n\n**Instance Type:** ${instanceType}`
156+
baseTooltip = `**Space:** ${spaceName} \n\n**Application:** ${appType} \n\n**Instance Type:** ${instanceType}`
157+
if (remoteAccess === RemoteAccess.ENABLED) {
158+
baseTooltip += `\n\n**Remote Access:** Enabled`
159+
} else if (remoteAccess === RemoteAccess.DISABLED) {
160+
baseTooltip += `\n\n**Remote Access:** Disabled`
161+
}
162+
163+
return baseTooltip
153164
}
154165
return `**Space:** ${spaceName} \n\n**Application:** ${appType} \n\n**Domain ID:** ${domainId} \n\n**User Profile:** ${owner}`
155166
}
@@ -166,20 +177,12 @@ export class SagemakerSpace {
166177

167178
public getContext(): string {
168179
const status = this.getStatus()
169-
if (status === 'Running' && this.spaceApp.SpaceSettingsSummary?.RemoteAccess === 'ENABLED') {
170-
return 'awsSagemakerSpaceRunningRemoteEnabledNode'
171-
} else if (status === 'Running' && this.spaceApp.SpaceSettingsSummary?.RemoteAccess === 'DISABLED') {
172-
return 'awsSagemakerSpaceRunningRemoteDisabledNode'
173-
} else if (status === 'Running' && this.isSMUSSpace) {
180+
181+
// only distinguish between running and non-running states
182+
if (status === SpaceStatus.RUNNING) {
174183
return 'awsSagemakerSpaceRunningNode'
175-
} else if (status === 'Stopped' && this.spaceApp.SpaceSettingsSummary?.RemoteAccess === 'ENABLED') {
176-
return 'awsSagemakerSpaceStoppedRemoteEnabledNode'
177-
} else if (
178-
(status === 'Stopped' && !this.spaceApp.SpaceSettingsSummary?.RemoteAccess) ||
179-
this.spaceApp.SpaceSettingsSummary?.RemoteAccess === 'DISABLED'
180-
) {
181-
return 'awsSagemakerSpaceStoppedRemoteDisabledNode'
182184
}
185+
183186
return this.isSMUSSpace ? 'smusSpaceNode' : 'awsSagemakerSpaceNode'
184187
}
185188

0 commit comments

Comments
 (0)