Skip to content

Conversation

@ellisonbg
Copy link
Collaborator

@ellisonbg ellisonbg commented May 14, 2025

This integrates the OutputProcessor into the kernel client. The result is that output messages are now written to the server-side ydoc rather than sent to the frontend directly (if the kernel session shows it is a notebook). Most of the logic is in the OutputProcessor and OutputsManager classes which were merged previously. @3coins

Copy link
Collaborator

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Thanks for opening this Brian! Can't comment on the core logic since it's outside my scope, but I left some code suggestions below. These are non-blocking, feel free to ignore

outputs_manager = Instance(
klass=OutputsManager,
help="An instance of the OutputsManager",
allow_none=True
).tag(config=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be a trait? It seems like this can be defined as a property:

Suggested change
outputs_manager = Instance(
klass=OutputsManager,
help="An instance of the OutputsManager",
allow_none=True
).tag(config=True)
@property
def outputs_manager(self) -> OutputsManager:
return self.settings["outputs_manager"]

This has the added benefit of clearly distinguishing the two attributes:

  • outputs_manager_class: a configurable class trait
  • outputs_manager: a property on RtcExtensionApp, set by the extension itself

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same goes for yroom_manager above.

@ellisonbg ellisonbg force-pushed the output-integration branch from 4f414d4 to 6bd1915 Compare May 15, 2025 23:32
@ellisonbg ellisonbg changed the title Integrate outputs manager into extension app. Integrate outputs manager into extension app May 16, 2025
@ellisonbg ellisonbg force-pushed the output-integration branch from bbf4235 to 783e29d Compare May 16, 2025 16:27
@Zsailer Zsailer merged commit 94e7a41 into main May 16, 2025
2 of 6 checks passed
@Zsailer Zsailer deleted the output-integration branch May 16, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants