KTNB-1205: Support Jupyter Widgets #4
Conversation
ileasile
force-pushed
the
ktnb-1205
branch
2 times, most recently
from
3f9dcb4 to
48934a2
Compare
|
@codex Please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val wireMessage = getWireMessage(fullStates) | ||
| val message = UpdateStatesMessage(wireMessage.state, wireMessage.bufferPaths) | ||
|
|
||
| val data = Json.encodeToJsonElement<WidgetMessage>(message).jsonObject | ||
| comm.send(data, null, emptyList()) |
There was a problem hiding this comment.
When handling RequestStatesMessage, the code builds a WireMessage that includes buffer payloads but sends the update with emptyList() for the buffers, so any widget state containing binary values (e.g., ByteArray fields) will lose its data when the frontend requests all states over jupyter.widget.control. This leaves the client with incomplete state and breaks widgets that rely on buffered values; the buffers from wireMessage need to be forwarded.
Useful? React with 👍 / 👎.
| require(pathEl is JsonArray) { "Expected JSON array inside buffer_paths, got: $pathEl" } | ||
| pathEl.map { element -> | ||
| element as? JsonPrimitive ?: error("Expected JSON primitive inside buffer_paths, got: $element") | ||
| if (element.isString) return@map element.content | ||
| element.doubleOrNull ?: element.intOrNull ?: error("Unsupported buffer path element: $element") |
There was a problem hiding this comment.
BufferPathsSerializer.deserialize tries doubleOrNull before intOrNull, so numeric path elements come back as Double (e.g., 0.0). getPatch then casts list indices to Int, which will crash with a ClassCastException for any buffer path that contains a list index (common when buffers sit inside arrays). Parsing integers first avoids converting list indices to doubles and prevents the crash.
Useful? React with 👍 / 👎.
|
@codex I tried to fix these obvious bugs. Could you please give me more deep, architectural insights? |
|
Summary
Testing
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Suppress("UNCHECKED_CAST") | ||
| override fun deserialize( | ||
| patchValue: Any?, | ||
| widgetManager: WidgetManager, | ||
| ): T = patchValue as T |
There was a problem hiding this comment.
Float-based widget properties are deserialized via PrimitiveWidgetModelPropertyType.deserialize, which simply casts the incoming patch value to T. The JSON parser first returns Int for whole-number literals (see deserializePrimitive in protocol/JsonSerialization.kt), so a float widget receiving an update like { "value": 1 } will pass an Int into this cast and throw ClassCastException before the patch is applied. Any widget with FloatType fields (e.g., float sliders) will crash whenever the frontend sends an integer-valued update. Consider normalizing numeric JSON to the property type (e.g., coercing numbers to Double) before casting.
Useful? React with 👍 / 👎.
|
@codex review |
ileasile
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override fun serialize( | ||
| propertyValue: M, | ||
| widgetManager: WidgetManager, | ||
| ): String = "$WIDGET_REF_PREFIX${widgetManager.getWidgetId(propertyValue)}" | ||
|
|
There was a problem hiding this comment.
Serializing widget references interpolates getWidgetId(propertyValue) without checking that the widget has actually been registered, so an unregistered widget set on a property is emitted as IPY_MODEL_null. The frontend will never be able to resolve that model id and the reference silently breaks instead of throwing or registering the widget. Consider failing fast (or auto-registering) when getWidgetId returns null to avoid emitting invalid widget references.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Reflect kernel-side changes on the frontend | ||
| widget.addChangeListener { patch -> | ||
| val wireMessage = getWireMessage(patch) |
There was a problem hiding this comment.
The change listener sends a WidgetUpdateMessage for every patch on a model, but comm.onMessage later applies patches coming from the frontend using applyPatch, which sets the same properties and triggers this listener. As a result, every frontend update is immediately echoed back as an update message instead of being handled silently (or using the dedicated echo_update path), creating redundant traffic and risking feedback loops when multiple views are attached to the same widget. Filtering out patches that originate from the frontend before sending an update would prevent this echo.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val widget = widgetFactory.create(this) | ||
| val patch = openMessage.toPatch(buffers) | ||
| widget.applyFrontendPatch(patch) |
There was a problem hiding this comment.
When handling a jupyter.widget comm open initiated by the frontend, the manager eagerly constructs the widget with widgetFactory.create(this) before applying the incoming state. The generated widget constructors register their default dependencies (e.g., IntSliderWidget’s layout and style defaults call widgetManager.layout()/sliderStyle() which immediately send comm opens). As a result every frontend-originated comm open will spuriously register and open extra widget models on the frontend even though that state is already owned by the sender, leading to duplicated model IDs and unexpected comm traffic. Consider constructing the widget without registering defaults (or deferring default registration) when hydrating from a frontend open message.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val specName = "${info.functionName}Spec" | ||
| builder.appendLine("private val $specName = WidgetSpec(") | ||
| builder.appendLine(" modelName = \"${info.schema.model.name}\",") | ||
| builder.appendLine(" modelModule = \"${info.schema.model.module}\",") | ||
| builder.appendLine(" modelModuleVersion = \"${info.schema.model.version}\",") | ||
| builder.appendLine(" viewName = \"${info.schema.view.name}\",") | ||
| builder.appendLine(" viewModule = \"${info.schema.view.module}\",") | ||
| builder.appendLine(" viewModuleVersion = \"${info.schema.view.version}\",") |
There was a problem hiding this comment.
The generator writes viewName = "${info.schema.view.name}" without guarding for schemas whose view name is null (e.g., LinkModel, DirectionalLinkModel, DOMWidgetModel in schema.json). The generated specs therefore embed the literal string "null" in _view_name (see LinkWidget.kt/DOMWidget.kt), so when these widgets are registered the frontend receives _view_name: "null" and cannot resolve a view class, leaving the widget unrenderable. The generator should omit or default the view name instead of stringifying null.
Useful? React with 👍 / 👎.
| @Suppress("UNCHECKED_CAST") | ||
| override var value: T | ||
| get() = _value as T |
There was a problem hiding this comment.
Widget property delegates cast _value to a non-null type with get() = _value as T, but several generated widgets initialize required references to null when created from the frontend (e.g., layout/style use widgetProp(..., if (fromFrontend) null else …)). If the comm open payload omits those fields—which is allowed—the property stays null and any later access or getFullState (e.g., replying to request_state) will throw an NPE here. Frontend-originated widgets thus crash as soon as their state is read unless the code permits null or supplies a default instance.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dependencies { | ||
| compileOnly(libs.kotlinx.serialization.json) | ||
| implementation(libs.kotlin.reflect) |
There was a problem hiding this comment.
widgets-api declares kotlinx-serialization-json as compileOnly even though runtime code (e.g., WidgetManagerImpl/BufferPathsSerializer) calls Json.decodeFromJsonElement and Json.encodeToJsonElement while handling comm traffic. Any consumer that depends on widgets-api/widgets-jupyter without manually adding kotlinx-serialization-json will hit NoClassDefFoundError as soon as a widget is instantiated or a comm message is processed. This should be an implementation/api dependency so the runtime library is on the classpath.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name = "step", | ||
| default = 60, | ||
| serializerSelector = { value -> | ||
| when (value) { | ||
| is Double -> FloatType |
There was a problem hiding this comment.
The generated StepUnionType for TimeWidget only routes Double values through FloatType; every other value falls through to the enum branch. Because the default step is 60 (an Int), serialization immediately treats it as a WidgetEnumEntry, leading to a ClassCastException when creating the widget or when users set the step to any integer. Integer step values should be handled as numbers rather than being cast to the enum branch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dependencies { | ||
| api(libs.kotlinx.serialization.json) | ||
| implementation(libs.kotlin.reflect) |
There was a problem hiding this comment.
The new widgets-api module imports Jupyter API types such as DisplayResult and CommManager (see WidgetManagerImpl.kt), but its build file never adds the Kotlin Jupyter API dependency or calls kotlinJupyter { addApiDependency() } the way other integrations do (e.g., http-util/ktor-client). With only serialization and reflect on the classpath, these imports will be unresolved and the module will not compile. Please add the kotlin-jupyter API dependency to this module (and transitively for widgets-jupyter/tests) so the code can build.
Useful? React with 👍 / 👎.
…try` for improved modularity.
…y and simplify factory creation.
…odel, get rid of ugly ID registration
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dependencies { | ||
| testImplementation(projects.integrations.widgets.widgetsJupyter) | ||
| testImplementation(libs.kotlin.test) | ||
| testImplementation(libs.test.kotlintest.assertions) |
There was a problem hiding this comment.
The new WidgetReplTest imports JupyterReplTestCase, CommManagerImpl, and other kotlin-jupyter classes (e.g., lines 19–21 of WidgetReplTest.kt), but widgets-tests only depends on widgets-jupyter, kotlin-test, and kotest. None of these bring in the kotlin-jupyter test kit or protocol implementations that define those symbols, so the test module will not compile. Add the appropriate kotlin-jupyter testkit/lib dependency to the test configuration so the test sources resolve.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
These dependencies are added automatically
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private fun initializeWidget( | ||
| comm: Comm, | ||
| widget: WidgetModel, | ||
| ) { | ||
| val modelId = comm.id | ||
| widgetIdByWidget[widget] = modelId | ||
| widgets[modelId] = widget | ||
|
|
||
| // Reflect kernel-side changes on the frontend | ||
| widget.addChangeListener { patch, fromFrontend -> | ||
| handleWidgetUpdate(comm, widget, patch, fromFrontend) | ||
| } | ||
|
|
||
| // Reflect frontend-side changes on kernel | ||
| comm.onMessage { msg, _, buffers -> | ||
| when (val message = Json.decodeFromJsonElement<WidgetMessage>(msg)) { | ||
| is WidgetStateMessage -> { | ||
| widget.applyFrontendPatch(message.toPatch(buffers)) | ||
| } | ||
|
|
||
| is RequestStateMessage -> { | ||
| val fullState = widget.getFullState() | ||
| val wireMessage = getWireMessage(fullState) | ||
| val data = | ||
| Json | ||
| .encodeToJsonElement<WidgetMessage>( | ||
| WidgetUpdateMessage( | ||
| wireMessage.state, | ||
| wireMessage.bufferPaths, | ||
| ), | ||
| ).jsonObject | ||
| comm.send(data, null, wireMessage.buffers) | ||
| } | ||
|
|
||
| is CustomMessage -> {} | ||
|
|
||
| else -> {} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Widgets are added to widgets and widgetIdByWidget inside initializeWidget, but the comm lifecycle never removes them when the frontend closes the comm. If a widget is disposed in the UI and the kernel remains running (e.g., multiple widget lifecycles in one notebook), the manager will keep stale models and will continue to include them in request_states responses and change notifications, leaking memory and potentially sending updates to closed comms. Consider registering an onClose handler that removes the widget from both maps to keep the registry consistent.
Useful? React with 👍 / 👎.
…eaks and ensure proper cleanup.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serializerSelector = { value -> | ||
| when (value) { | ||
| is Number -> FloatType | ||
| is WidgetEnumEntry<*> -> WidgetEnumType(Step, Step.Any) | ||
| else -> WidgetEnumType(Step, Step.Any) |
There was a problem hiding this comment.
step fails for string values
The union serializer for the step property routes any non‑numeric value to WidgetEnumType, which only accepts WidgetEnumEntry. If a user follows the docs and sets timeWidget.step = "any" (string), the setter stores the string, the change listener invokes WidgetEnumType.serialize, and a ClassCastException is thrown because the string is not converted to Step.Any. The same union is used by DatePickerWidget, so both widgets crash when step is set to a string value. The serializer needs to map strings to the enum entry (or reject them up front) before serialization.
Useful? React with 👍 / 👎.
…erty type handling, remove redundant enum, and update related tests and logic.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...gets-api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/widget/model/WidgetFactoryRegistry.kt
Show resolved
Hide resolved
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private val widgetOpenMetadataJson = | ||
| buildJsonObject { | ||
| put("version", "${DEFAULT_MAJOR_VERSION}.${DEFAULT_MINOR_VERSION}") |
There was a problem hiding this comment.
Comm opens currently advertise version as "2.0", but the Jupyter widgets protocol expects a full semver string such as 2.0.0 (ipywidgets’ PROTOCOL_VERSION is 2.0.0). Front-ends validate this metadata with @jupyter-widgets/base’s semver checks, and an invalid version string will cause the comm open to be rejected so kernel-created widgets never render. Please send a valid three-part version in the comm metadata.
Useful? React with 👍 / 👎.
…idgets, update `WidgetManager` and `WidgetModel` APIs, refactor tests to include new message handling logic.
…pdate default minor version, and refactor version metadata logic.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val version = widget.modelModuleVersion | ||
| val matchResult = versionConstraintRegex.find(version) | ||
| if (matchResult != null) { | ||
| versionMajor = matchResult.groupValues[1].toInt() | ||
| versionMinor = matchResult.groupValues[2].toInt() |
There was a problem hiding this comment.
The renderWidget MIME bundle currently derives version_major/version_minor from _model_module_version (lines 128‑132), but the Jupyter widget view payload is supposed to advertise the view module version (_view_module_version). For widgets where the view bundle has a different version than the model bundle (common for third‑party widgets), this code will emit the wrong version numbers and the front‑end will reject the view, leaving the widget unable to render even though the correct view assets are available.
Useful? React with 👍 / 👎.
| ): WidgetFactory<*> = | ||
| factoryCache.getOrPut(modelName) { | ||
| ServiceLoader | ||
| .load(WidgetFactory::class.java, classLoader) | ||
| .firstOrNull { it.spec.modelName == modelName } ?: error("No factory for model $modelName") |
There was a problem hiding this comment.
Factory resolution is cached solely by model name (getOrPut on factoryCache) without considering the classloader that ServiceLoader should use (lines 19‑23). WidgetJupyterIntegration updates the classloader after every cell, but once a factory is cached it will never be reloaded from a newer classloader, so adding or reloading a widget library later in the session will keep using the stale factory and can trigger ClassNotFoundException or outdated code until the kernel restarts. The cache needs to be keyed by classloader or invalidated when the provider changes.
Useful? React with 👍 / 👎.
…on` instead of `modelModuleVersion` during version parsing.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…tion to be more specific
… notebook. Added TODO notes where-ever something is not yet working as expected
…numeric buffer path elements, remove unused `doubleOrNull`.
…t code style and configuration updates.
…okTests` to clean up deprecated code.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
1 similar comment
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
In this PR:
Comes next: