Skip to content

Conversation

@dlqqq
Copy link
Collaborator

@dlqqq dlqqq commented Jun 23, 2025

Description

  • Makes YRoomManager a logging configurable, set by the ServerDocsApp.yroom_manager_class trait.

  • Makes YRoom a logging configurable, set by the YRoomManager.yroom_class trait.

  • YRoomFileAPI, YRoomEventsAPI, and YjsClientGroup have all been made logging configurable, set by the YRoom.file_api_class, YRoom.events_api_class, and YRoom.client_group_class traits respectively.

  • Adds the YRoomFileAPI.poll_interval float trait, which configures how frequently the YRoomFileAPI writes unsaved changes to disk, checking for in-band & out-of-band changes before doing so.

  • Fixes a regression introduced by Allow YRoom to be restarted and allow multi-room kernels #117, where the server extension would not shut down properly.

Technical details

#117 refactored the YRoom task to make its stop() method fully synchronous. Each room's content would be saved in a background task. Since YRoom.stop() & YRoomManager.stop() were made synchronous, the ServerDocsApp.stop_extension() method was also made synchronous.

However, that change led to a race condition where the ContentsManager shut down before each room's content was saved to disk. The regression has been fixed by adding a YRoom.until_saved awaitable property that is only awaited when the server shuts down. The ServerDocsApp.stop_extension() method is now asynchronous again.

Reviewer guidance

  • Verify that the configurable traits work
  • Verify that the server extension stops gracefully
  • Verify file contents are saved after the server extension stops

@dlqqq dlqqq changed the title Fix server extension stopping, migrate YRoom classes to traitlets Migrate YRoom classes to traitlets, fix server extension stopping Jun 23, 2025
@dlqqq dlqqq added enhancement New feature or request bug Something isn't working labels Jun 23, 2025
@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 23, 2025

@Zsailer Thank you for the feedback. I've gone through the logging configurables added by this PR and drastically simplified all of their constructors. Each only takes 1-2 arguments now. 👀

However, I wasn't able to remove every __init__() or make every instance attribute a trait because there are scenarios where traitlets breaks type safety. Here is what happened when I turned YRoom.file_api into a traitlets.Instance trait:

Screenshot 2025-06-23 at 10 38 50 AM

Even though I had specified klass=YRoomFileAPI, the type checker is unable to resolve the type of instance traits. I believe this is the same issue causing self.log to be unresolved in LoggingConfigurable instances.

I don't think that using traitlets should come at the cost of type safety, especially if instance attributes work for now & there is not a clear need to use private traits. I've opened a follow-up issue for more discussion & to allow these changes to be addressed in a future PR: #123.

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 23, 2025

Same issue is occurring on ServerDocsApp.yroom_manager after making it an Instance trait:

Screenshot 2025-06-23 at 3 07 11 PM

Copy link
Collaborator

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thanks @dlqqq! Great work here. Let's merge and use #123 for further discussion/iteration. I think any further pushing on traitlets is low priority :) We're getting configuration here that we want, and that's a big win!

@Zsailer Zsailer merged commit 8aa29e1 into jupyter-ai-contrib:main Jun 23, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants