Skip to content
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

Better support when multiple renderers are installed #125876

Closed
miguelsolorio opened this issue Jun 9, 2021 · 26 comments
Closed

Better support when multiple renderers are installed #125876

miguelsolorio opened this issue Jun 9, 2021 · 26 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook-output papercut 🩸 A particularly annoying issue impacting someone on the team verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@miguelsolorio
Copy link
Contributor

Steps to Reproduce:

  1. Install the Jupyter extension
  2. Install another notebook extension w/ a custom renderer (httpBook)
  3. Open a file from custom renderer (.http), everything looks fine
    CleanShot 2021-06-09 at 13 56 27@2x
  4. Open a Jupyter notebook (.ipynb)
  5. 🐛 all outputs have defaulted to the custom renderer
    CleanShot 2021-06-09 at 13 58 10@2x
    CleanShot 2021-06-09 at 13 58 19@2x

I would expect that a custom renderer that only works with certain filetypes (http) to only appear for those notebooks, not all notebooks. I'd also hope for an easy way to set the default renderer for certain notebook types.

@miguelsolorio miguelsolorio added bug Issue identified by VS Code Team member as probable bug notebook labels Jun 9, 2021
@rebornix rebornix added this to the June 2021 milestone Jun 9, 2021
@rebornix
Copy link
Member

rebornix commented Jun 9, 2021

Not sure yet if this should be a candidate, can't recall when/why the user setting to specify which extension/renderer has higher priority is gone.

@connor4312
Copy link
Member

This is actually working 'as designed'. The mimetype for httpbook's custom renderer is text/html. When the user selects that, we remember that they prefer that as their renderer for the given mimetype.

However maybe we should only remember that in the context of a single notebookType rather than globally.

@rebornix
Copy link
Member

rebornix commented Jun 9, 2021

The catch here is users are very likely unaware that this extension contributes text/html renderer and might think this is a bug. It's not visible anywhere in the notebook editor that you can switch mimetypes or renderers.

Also I'm wondering if some of the basic mime type renderers in the core should have higher priority than the custom ones. For example text/html is trivial and that's what users should get oob no matter if there is a renderer for text/html. Otherwise every extension will implement their own text/html renderer if they want the outputs rendering to be consistent for basic mime types.

When the user selects that, we remember that they prefer that as their renderer for the given mimetype

@connor4312 this is not working right now.

@rebornix rebornix added under-discussion Issue is under discussion for relevance, priority, approach and removed bug Issue identified by VS Code Team member as probable bug labels Jun 9, 2021
@connor4312
Copy link
Member

I think that's a good idea if we also have some notion of extension affinity, where a custom renderer for a built-in mimetype is preferred if the output was created by the same extension who contributed the renderer. If this is not the case, then it's a poor experience for notebooks that intend to have a custom renderer, since their users will manually need to switch their renderer.

Or we just disallow people overwriting built-in mimetypes, and say to use a custom mimetype instead.

@rebornix
Copy link
Member

rebornix commented Jun 9, 2021

Thought about this again, as a renderer author, my renderer offers rich rendering for application/vnd.code.notebook.error, which is also builtin. I personally would expect the custom renderer to be the default renderer for application/vnd.code.notebook.error in all types of notebooks (if it's the only custom one).

Considering how I built the renderer, I don't think we can disallow people overwriting builtin mimetypes.

@tanhakabir
Copy link
Contributor

Yeah I also don't like disallowing overriding builtin mimetypes but we can say that the default builtin mimetype is the preferred one unless the user selects a different one for the viewtype or the renderer extension itself contributed the renderer

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jun 9, 2021

one unless the user selects a different one for the viewtype or the renderer extension itself contributed the renderer

What happens if I select XYZ renderer for rendering HTML when looking at Notebook ABC (where both were contributed by two seprate extensions). Does this mean that XYZ will be used to render HTML output in httpbook?

The user may not expect that to happen.

@connor4312
Copy link
Member

What happens if I select XYZ renderer for rendering HTML when looking at Notebook ABC (where both were contributed by two seprate extensions). Does this mean that XYZ will be used to render HTML output in httpbook?

This would be in addition to the notebookType association, so it would only set it in ABC.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jun 9, 2021

So, something like a custom renderer for application/vnd.code.notebook.error would only apply for a specific notebook type. is that correct?

@tanhakabir
Copy link
Contributor

@nat1craft mentioned in REST Book that the last output renderer he selected isn't the default in the next cell run. @connor4312 is this expected?

@AnWeber
Copy link
Contributor

AnWeber commented Jun 25, 2021

@tanhakabir I can confirm that the selected editor is not the default for the next run.

I saw the separation between Notebook and NotebookRenderer and the possibility of different renderers for the same Mimetype as a feature. One use case for this would be rendering application/geo+json. Partly I want to see the representation as JSON, partly I want to view the data in a map view. However, I would leave it up to the user to decide how the default display should be (#123884 (comment)).

@rebornix rebornix modified the milestones: June 2021, July 2021 Jul 1, 2021
@RandomFractals
Copy link

yep. you should try this too: RandomFractals/vscode-data-table#23 (comment)

data-table-chicago-traffic

@RandomFractals
Copy link

RandomFractals commented Jul 6, 2021

@connor4312 this might require further investigation on your end for CSV data loading and renderers with support for mulitple mime types, csv and json in my prototype ... tanhakabir/rest-book#114

@Pit-Storm
Copy link

I don't really now if this is correct here, but this is the only issue that is related to the problem I am facing with.

When I am using jupyter notebooks and there should be an image displayed in the output of a cell, there is only showed e.g. <Figure size 432x288 with 1 Axes>. To display the actual image I have to click on </> next to the output region and select another renderer.

My point is, that as a user, I have to choose that different renderer over and over again for every cell. This is really slowing down developing.

I tried to search for a solution, but I wasn't able to find something about a default renderer in the settings.

Maybe here someone could help solving that problem (or it gives you a new hint for fixing it in general) :-)


Specs:
Version: 1.61.2 (system setup)
Commit: 6cba118
Electron: 13.5.1
Chrome: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Windows_NT x64 10.0.19042

Jupyter Extension: v2021.9.1101343141
Python: 3.8.10

@rebornix rebornix removed the notebook label Oct 21, 2021
@connor4312
Copy link
Member

connor4312 commented Oct 21, 2021

I think we should take care of this, it's a large paper cut. So how about this algorithm, for any given output with mimetype X:

  • If the user has explicitly selected a renderer for X, in this notebook type and workspace, use that.
  • Otherwise, if the extension providing the notebook also provides a renderer for X, use that.
  • Otherwise, if there is a built-in renderer for X, use that. (From previous comments)
  • Otherwise, if there is another extension who renders mimetype X, use that.

Whenever a user changes the output renderer, persist the preference to use that renderer for mimetype X within the current notebook type in workspace storage. (Effectively Map<NotebookType, Map<Mimetype, RendererId>>)

@connor4312 connor4312 added feature-request Request for new features or functionality papercut 🩸 A particularly annoying issue impacting someone on the team labels Oct 21, 2021
@RandomFractals
Copy link

@connor4312 I really like the proposed renderer selection strategy. I think it will make it work much better for the couple of simple data renders I've created for basic data and map views, without requiring to change default on every cell run.

Thank you! I look forward to this implementation and release and open to assist with testing of this feature in insiders.

@JacksonKearl
Copy link
Contributor

@connor4312 what is rationale for saving this in a workspace scope versus the global scope? Extensions, mimetypes, and notebook types are all globally scoped entities, so to me it seems unexpected that I'd be configuring them at the workspace scope (and thus needing to reconfigure them for each new workspace, and have potential weirdness in empty workspaces, multi-root, opening a super/sub-folder of an existing workspace, etc.)

I might suggest:

  • If the user has explicitly selected a renderer for X, in this notebook type and workspace, use that.
  • ➕ If the user has most recently selected renderer R for mimetype X in this notebook type, use that.
  • Otherwise, if the extension providing the notebook also provides a renderer for X, use that.
  • Otherwise, if there is a built-in renderer for X, use that. (From previous comments)
  • Otherwise, if there is another extension who renders mimetype X, use that.

@connor4312
Copy link
Member

connor4312 commented Oct 22, 2021

The rationale is if you're working with different types of data in different places and want to use different renderers for them. For example, if the user selects notebook Y's custom html renderer for a notebook in one workspace, but wants the ordinary renderer elsewhere. While these are all global things, the context in which I want to use any combination of could be local. It would be inconvenient to have to switch back and forth between them as you go from one workspace to the other.

@connor4312 connor4312 added verification-needed Verification of issue is requested and removed under-discussion Issue is under discussion for relevance, priority, approach labels Oct 26, 2021
@rchiodo rchiodo added the verified Verification succeeded label Oct 26, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Oct 26, 2021

I think I verified this. Having a hard time getting two different custom notebook types to output the same type.

@RandomFractals
Copy link

RandomFractals commented Oct 27, 2021

@rchiodo I believe you already check it out, but you can use my notebook examples from the simple data table renderer: https://github.com/RandomFractals/vscode-data-table

@connor4312 thanks for making those changes to support workspace and individual notebook cell output rendrerer preferences.

@rchiodo
Copy link
Contributor

rchiodo commented Oct 27, 2021

Thanks @RandomFractals. I don't believe this is behaving as expected then.

I open a rest notebook, run a cell, chose the data table renderer and get this:

image

I open an http notebook, run a cell, but don't choose anything and I get the renderer from the other notebook being used

image

image

@connor4312 is that the expected behavior? Maybe I'm misinterpreting the algorithm.

@connor4312
Copy link
Member

@rchiodo what extensions are you using for the two different notebooks?

@rchiodo
Copy link
Contributor

rchiodo commented Oct 27, 2021

First one has the rest notebook extension installed to run it and the 'Data Table renderer' to render it.
Second one has the http notebook extension installed.

@rchiodo
Copy link
Contributor

rchiodo commented Oct 27, 2021

Here's the exact extension information:

Name: REST Book
Id: tanhakabir.rest-book
Description: Notebook for running REST queries.
Version: 4.2.0
Publisher: Tanha Kabir
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=tanhakabir.rest-book

Name: Data Table
Id: randomfractalsinc.vscode-data-table
Description: Data Table 🈸 renderer for Notebook 📓 cell ⌗ data outputs
Version: 1.8.0
Publisher: Random Fractals Inc.
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=RandomFractalsInc.vscode-data-table

Name: httpBook - Rest Client
Id: anweber.httpbook
Description: Quickly and easily send REST, Soap, GraphQL, GRPC, MQTT and WebSocket requests directly within Visual Studio Code
Version: 2.1.1
Publisher: Andreas Weber
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=anweber.httpbook

@connor4312
Copy link
Member

Fixed: the 3rd and 4th choices were swapped (built-in renderers were not chosen by default)

@rebornix rebornix removed their assignment Oct 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook-output papercut 🩸 A particularly annoying issue impacting someone on the team verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

11 participants