Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 58 additions & 21 deletions packages/core/src/awsService/sagemaker/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,32 +191,69 @@ export async function openRemoteConnect(
void vscode.window.showErrorMessage(ConnectFromRemoteWorkspaceMessage)
return
}
await tryRefreshNode(node)
if (node.getStatus() === 'Stopped') {
// In case of SMUS, we pass in a SM Client and for SM AI, it creates a new SM Client.
const client = sageMakerClient ? sageMakerClient : new SagemakerClient(node.regionCode)

try {
await client.startSpace(node.spaceApp.SpaceName!, node.spaceApp.DomainId!)
await tryRefreshNode(node)
const appType = node.spaceApp.SpaceSettingsSummary?.AppType
if (!appType) {
throw new ToolkitError('AppType is undefined for the selected space. Cannot start remote connection.', {
code: 'undefinedAppType',
})
}
await client.waitForAppInService(node.spaceApp.DomainId!, node.spaceApp.SpaceName!, appType)
await tryRemoteConnection(node, ctx)
} catch (err: any) {
// Ignore InstanceTypeError since it means the user decided not to use an instanceType with more memory
if (err.code !== InstanceTypeError) {

const spaceName = node.spaceApp.SpaceName!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be a nullable, are we sure all the functions below which use this field handle the null case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will happen if this is null/undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaceName cannot be null because spaces must have names when created


try {
await tryRefreshNode(node)

// for Stopped SM spaces - check instance type before showing progress
if (node.getStatus() === 'Stopped') {
// In case of SMUS, we pass in a SM Client and for SM AI, it creates a new SM Client.
const client = sageMakerClient ? sageMakerClient : new SagemakerClient(node.regionCode)

try {
Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Nov 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sry what's the purpose of having 2 layers of try...catch?
cant' we use 1?

try {
} catch() {
    if (error.code === InstanceTypeError) {
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it.

Copy link
Contributor

@dylanraws dylanraws Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears previously we were logging all uncaught errors. Do we no longer want to do that? I just want to make sure removing that was intentional.

getLogger().error(`sm:openRemoteConnect: ${err}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not there before more change. First I thought of having uncaught errors throws as well.
Now with this change only Progress Indicator is in place

await client.startSpace(spaceName, node.spaceApp.DomainId!)
await tryRefreshNode(node)
const appType = node.spaceApp.SpaceSettingsSummary?.AppType
if (!appType) {
throw new ToolkitError(
'AppType is undefined for the selected space. Cannot start remote connection.',
{
code: 'undefinedAppType',
}
)
}

// Only start showing progress after instance type validation
return await vscode.window.withProgress(
{
location: vscode.ProgressLocation.Notification,
cancellable: false,
title: `Connecting to ${spaceName}`,
},
async (progress) => {
progress.report({ message: 'Starting the space.' })
await client.waitForAppInService(node.spaceApp.DomainId!, spaceName, appType)
await tryRemoteConnection(node, ctx, progress)
}
)
} catch (err: any) {
// Ignore InstanceTypeError since it means the user decided not to use an instanceType with more memory
// just return without showing progress
if (err.code === InstanceTypeError) {
return
}
throw new ToolkitError(`Remote connection failed: ${(err as Error).message}`, {
cause: err as Error,
code: err.code,
})
}
} else if (node.getStatus() === 'Running') {
// For running spaces, show progress
return await vscode.window.withProgress(
{
location: vscode.ProgressLocation.Notification,
cancellable: false,
title: `Connecting to ${spaceName}`,
},
async (progress) => {
await tryRemoteConnection(node, ctx, progress)
}
)
}
} else if (node.getStatus() === 'Running') {
await tryRemoteConnection(node, ctx)
} catch (err: any) {
getLogger().error(`sm:openRemoteConnect: ${err}`)
throw err
}
}
4 changes: 3 additions & 1 deletion packages/core/src/awsService/sagemaker/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@ const logger = getLogger('sagemaker')

export async function tryRemoteConnection(
node: SagemakerSpaceNode | SagemakerUnifiedStudioSpaceNode,
ctx: vscode.ExtensionContext
ctx: vscode.ExtensionContext,
progress: vscode.Progress<{ message?: string; increment?: number }>
) {
const spaceArn = (await node.getSpaceArn()) as string
const isSMUS = node instanceof SagemakerUnifiedStudioSpaceNode
const remoteEnv = await prepareDevEnvConnection(spaceArn, ctx, 'sm_lc', isSMUS, node)
try {
progress.report({ message: 'Opening remote session' })
await startVscodeRemote(
remoteEnv.SessionProcess,
remoteEnv.hostname,
Expand Down
Loading