-
Notifications
You must be signed in to change notification settings - Fork 6
Improved outputs handling when reading/writing from/to disk #125
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.
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
Conversation
|
@dlqqq This mostly works, but I am running into an exception by pycrdt when I try to make the changes to the YDoc. The transaction if failing saying the Ydoc isn't part of a room yet. My sense is that there is a race condition between the YRoom creation/initialization and document loading in this case. No hurry on this, but you are probably the right person to dive into this. |
dlqqq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellisonbg This PR looks great! Left some suggestions below.
I wasn't able to reproduce the race condition you're describing on 6 different notebooks. I don't see how this PR could introduce a race condition. The OutputsManager.load_notebook() method is synchronous, so adding a call to that method shouldn't change anything regarding concurrency.
|
Here is the exception I am seeing: The notebook I am trying is this one: |
|
OK, this is updated and ready for review. |
dlqqq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellisonbg Thank you for fixing that bug you found!
I've approved to unblock, but it would be great if you could downgrade the critical log & remove the asyncio.Event added in yroom_file_api.py before merging. 👍
|
|
||
| def process_saving_notebook(self, nb: dict) -> dict: | ||
| """ | ||
| Process a notebook before saving to disk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about: Mutate notebook dict in place, return updated value.
As for the "before saving" usage info, how about renaming the method to process_notebook_for_saving_to_disk ?
3coins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellisonbg
This looks great!, outputs work like magic now, I spent 5 mins looking for them in the notebook file 😆 .
Some minor comments to improve error handling and readability, but good overall.
| placeholder = self.write( | ||
| file_id, | ||
| cell_id, | ||
| output, | ||
| display_id, | ||
| asdict=True, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add error handling here?
| placeholder = self.write( | |
| file_id, | |
| cell_id, | |
| output, | |
| display_id, | |
| asdict=True, | |
| ) | |
| try: | |
| placeholder = self.write( | |
| file_id, | |
| cell_id, | |
| output, | |
| display_id, | |
| asdict=True, | |
| ) | |
| except Exception e: | |
| self.log.warning(f"Failed to write output for cell {cell_id}: {e}") | |
| placeholder = output |
|
OK, this is ready for final review. |
|
Looks good! |
|
I found a few more edge cases that I am fixing. |
|
@3coins can you have a look at my latest commit, review and merge (if ready)? |
This PR implements notebook output loading functionality that allows jupyter-server-documents to process and manage notebook outputs stored on disk rather than in the notebook file itself.
• Process outputs in notebooks on disk - Added core functionality to load notebook outputs from disk storage when notebooks are opened
• Enhanced OutputsManager - Extended manager with methods to process loaded and saving notebooks, including placeholder output handling and metadata management
• Improved placeholder handling - Better handling of placeholder outputs with support for both dict and Map formats
• Integration with YRoom system - Connected OutputsManager throughout the room management system for seamless output processing
Key Changes
Test Plan