Skip to content

Conversation

@3coins
Copy link
Collaborator

@3coins 3coins commented May 30, 2025

Fixes #69. This PR adds a new handler to load all cell outputs in one fetch call.

@3coins 3coins marked this pull request as draft May 30, 2025 16:00
@3coins 3coins force-pushed the all-outputs-for-cell branch from 244a556 to 0ad6fa5 Compare May 31, 2025 00:42
@3coins 3coins marked this pull request as ready for review May 31, 2025 00:56
@3coins
Copy link
Collaborator Author

3coins commented Jun 2, 2025

@ellisonbg
Updated the get_outputs method to sort by output index instead of modified time. Let me know if you have any other feedback.

Copy link
Collaborator

@ellisonbg ellisonbg left a comment

Choose a reason for hiding this comment

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

Minor changes requested.

@Zsailer
Copy link
Collaborator

Zsailer commented Jun 3, 2025

I agree with @ellisonbg's comments.

Otherwise, this PR looks good to me.

@3coins
Copy link
Collaborator Author

3coins commented Jun 3, 2025

@ellisonbg
Incorporated review comments, formatted the file.

@3coins 3coins force-pushed the all-outputs-for-cell branch from 523e3e3 to ed0f0c5 Compare June 3, 2025 18:06
@3coins 3coins force-pushed the all-outputs-for-cell branch from c532f97 to 04f1f71 Compare June 5, 2025 01:10
@dlqqq
Copy link
Collaborator

dlqqq commented Jun 5, 2025

@3coins had asked that I share feedback on the naming in this class based on Brian's most recent suggestion.

  • Since OutputsManager._create_outputs_link() never references self, it seems that this should live as a function in the outer scope like create_placeholder_output().

  • That method doesn't really create a link, but an output dictionary containing a link.

  • The word "placeholder" seems unnecessary all outputs are implied to be placeholders by design. That seems to be the purpose of this class.

  • Finally, the method & the function seem to serve a similar purpose but have entirely different arguments.

Therefore, I recommend renaming & refactoring:

  • OutputsManager._create_outputs_link(file_id, cell_id) => build_output_dict(file_id, cell_id, output_type)

    • This function can basically be implemented like create_placeholder_output() currently, but return a dict instead of a pycrdt.Map instead.
  • create_placeholder_output(output_type, url) => build_output_map(file_id, cell_id, output_type)

    • This function can call build_output_dict() and coerce it to pycrdt.Map.
    • I noticed the URL used in this function is slightly different, so you may need a stream: bool argument on both to switch between the two URL templates, or simply a url_template: str argument if you may need multiple.

I don't have much context on this area of the code, so please feel free to adapt these suggestions as you all see fit. 👋

@3coins
Copy link
Collaborator Author

3coins commented Jun 6, 2025

@dlqqq
Thanks for the feedback. I refactored the code around your suggestions, but kept the original naming for create_placeholder_output. Take a look at let me know if this can be improved further.

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.

New APIs look great! Approving to unblock as other changes can be made in future PRs are needed.

@3coins 3coins merged commit 401675f into jupyter-ai-contrib:main Jun 6, 2025
2 of 6 checks passed
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.

Add a new api to return all outputs for a cell

4 participants