Skip to content
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.

Sign up for GitHub

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

Connect refactor #4

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Connect refactor #4

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
21c62b2
move reusable grpc components into a base class
hotpocket Dec 5, 2024
1000516
Add ConnectSerializer and the code to select and run it. Refactor ker…
hotpocket Dec 5, 2024
c616b3c
handle error on missing tls similar to kernelServer. Add missing call…
hotpocket Dec 6, 2024
ea26204
add missing config block
hotpocket Dec 6, 2024
a302049
creating the maskedNotebook using `new` causes cells to be copied by …
hotpocket Dec 12, 2024
a9b0554
Merge branch 'main' into connect-refactor
hotpocket Dec 12, 2024
68ea437
getOutputsUri no longer resides on GrpcSerializer and thus broke this…
hotpocket Dec 12, 2024
49fe3a7
replace GrpcSerializer with ConnectSerializer and fix types as needed.
hotpocket Dec 12, 2024
827354a
client init in serializer.ts now calls `getTLSEnabled()` which needs`…
hotpocket Dec 12, 2024
052b70f
i have no idea what's going on here but this makes it work.
hotpocket Dec 12, 2024
10c9c8e
Merge branch 'main' into connect-refactor
hotpocket Dec 13, 2024
70fc30d
fix struct and data items that differ in new implementation.
hotpocket Dec 13, 2024
af02abc
fix typo
hotpocket Dec 13, 2024
81c8c03
fix invalid proto prop reference
hotpocket Dec 15, 2024
14b0f52
combine grpc base and grpc serializer. reorder changes to minimize PR…
hotpocket Dec 15, 2024
99964a2
more diff consolidation for PR
hotpocket Dec 15, 2024
f9f3e1b
pr diff cleanup
hotpocket Dec 17, 2024
f1e08e3
Merge branch 'main' into connect-refactor
hotpocket Dec 18, 2024
e4c2507
fix old schema via foyle npm update
hotpocket Dec 20, 2024
64f5b2b
Merge branch 'main' into connect-refactor
hotpocket Dec 20, 2024
1aa7dfe
expose TLS methods to avoid code duplication
hotpocket Dec 20, 2024
f9144e2
remove stale comment
hotpocket Dec 20, 2024
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
5 changes: 1 addition & 4 deletions src/extension/ai/converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import * as serializerTypes from '../grpc/serializerTypes'
import * as serializer from '../serializer'
import { Kernel } from '../kernel'

import * as protos from './protos'

// Converter provides converstion routines from vscode data types to protocol buffer types.
// It is a class because in order to handle the conversion we need to keep track of the kernel
// because we need to add execution information to the cells before serializing.
Expand All @@ -30,8 +28,7 @@ export class Converter {
let notebookDataWithExec = new vscode.NotebookData(cellDataWithExec)
// marshalNotebook returns a protocol buffer using the ts client library from buf we need to
// convert it to es
let notebookProto = serializer.GrpcSerializer.marshalNotebook(notebookDataWithExec)
return protos.notebookTSToES(notebookProto)
return serializer.GrpcSerializer.marshalNotebook(notebookDataWithExec)
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/extension/kernel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ import { handleCellOutputMessage } from './messages/cellOutput'
import handleGitHubMessage, { handleGistMessage } from './messages/github'
import { getNotebookCategories } from './utils'
import PanelManager from './panels/panelManager'
import { GrpcSerializer, SerializerBase } from './serializer'
import { GrpcSerializer } from './serializer'
import { askAlternativeOutputsAction, openSplitViewAsMarkdownText } from './commands'
import { handlePlatformApiMessage } from './messages/platformRequest'
import { handleGCPMessage } from './messages/gcp'
Expand Down Expand Up @@ -151,7 +151,7 @@ export class Kernel implements Disposable {
protected activeTerminals: ActiveTerminal[] = []
protected category?: string
protected panelManager: PanelManager
protected serializer?: SerializerBase
protected serializer?: GrpcSerializer
protected reporter?: GrpcReporter
protected featuresState$?: FeatureObserver

Expand Down
4 changes: 2 additions & 2 deletions src/extension/messages/platformRequest/saveCellExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Uri, env, workspace, commands } from 'vscode'
import { TelemetryReporter } from 'vscode-telemetry'
import getMAC from 'getmac'
import YAML from 'yaml'
import { FetchResult } from '@apollo/client'
import { FetchResult, MutationOptions } from '@apollo/client'

import { ClientMessages, NOTEBOOK_AUTOSAVE_ON, RUNME_FRONTMATTER_PARSED } from '../../../constants'
import { ClientMessage, FeatureName, IApiMessage } from '../../../types'
Expand Down Expand Up @@ -187,7 +187,7 @@ export default async function saveCellExecution(
},
},
}
const result = await graphClient.mutate(mutation)
const result = await graphClient.mutate(mutation as MutationOptions)
Copy link
Owner Author

Choose a reason for hiding this comment

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

I am concerned about this because I have no idea why this is needed. The typescript error on this was very hairy. If someone smarter than me knows what is going on here please let me know.

Copy link

Choose a reason for hiding this comment

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

Why would this code need to change at all if its running on head? What was the error you got?

Copy link
Owner Author

@hotpocket hotpocket Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  The types of 'variables.input.notebook.cells' are incompatible between these types.
    Type '{ outputs: { items: CellOutputItem[]; metadata: { [key: string]: string; }; processInfo?: CellOutputProcessInfo | undefined; }[]; kind: CellKind; ... 4 more ...; executionSummary?: CellExecutionSummary | undefined; }[]' is not assignable to type 'ReporterCellInput[]'.
      Type '{ outputs: { items: CellOutputItem[]; metadata: { [key: string]: string; }; processInfo?: CellOutputProcessInfo; }[]; kind: CellKind; ... 4 more ...; executionSummary?: CellExecutionSummary; }' is not assignable to type 'ReporterCellInput'.
        Types of property 'executionSummary' are incompatible.
          Type 'CellExecutionSummary | undefined' is not assignable to type 'InputMaybe<ReporterCellExecutionSummaryInput> | undefined'.
            Type 'CellExecutionSummary' is not assignable to type 'ReporterCellExecutionSummaryInput'.
              Types of property 'executionOrder' are incompatible.
                Type 'number | undefined' is not assignable to type 'InputMaybe<UInt32> | undefined'.
                  Type 'number' has no properties in common with type 'UInt32'.ts(2345)```

Copy link
Owner Author

@hotpocket hotpocket Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, after testing the wall thoroughly with my forehead I have found a loose brick. It seems this graphql stuff was generated and it seems from the timostam-ts proto type. On line 2014 in graphql.ts there exists a line executionOrder?: InputMaybe<UInt32>; which conforms to the timostam-ts type Notebook.Cell.CellExecutionSummary.executionOrder which is executionOrder?: UInt32Value; which is in contrast to the es type, which is, as you'd guess, a number ... so my guess is this graphql.ts needs to be regenerated.

Can you say more about what you mean by "running on head"? I am on this branch looking for issues caused by this code. I assume you mean the head of the upstream repo? If this is not correct please elaborate.

Copy link

Choose a reason for hiding this comment

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

Yes I mean making sure your branch is up to date with upstream changes

Copy link

Choose a reason for hiding this comment

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

Can this graphql code continue to use the timeostamp protos in this PR?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Can this graphql code continue to use the timeostamp protos in this PR?

This brings us back to the "can we make these two coexist" question as I see it, which was attempted for quite a long time until it was determined they could not coexist, and the original GrpcSerializer was modified so that there would not be two implementations. From what I can tell this will need to be regenerated, or we can force cast (as I have done in this branch) and pray it doesn't cause any runtime errors. I ran the tests for this and didn't get any failures, but I'm not sure this specific case is being tested.

I don't see where graphql.ts gets generated. Should I post a question on discord?

Copy link

Choose a reason for hiding this comment

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

Oh sorry I didn't realize from the diff this was using grpcSerializer. Yeah if its using grpcSerializer it might need to change. Sure ask in discord.

data = result
}
// TODO: Remove the legacy createCellExecution mutation once the reporter is fully tested.
Expand Down
Loading