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

lsmagic returns JSON which makes for awkward UX in JupyterLab #11793

Open
fperez opened this issue Jun 19, 2019 · 11 comments
Open

lsmagic returns JSON which makes for awkward UX in JupyterLab #11793

fperez opened this issue Jun 19, 2019 · 11 comments

Comments

@fperez
Copy link
Member

fperez commented Jun 19, 2019

I'm not sure why lsmagic needs to return a JSON data structure by default - but the end result is that in JupyterLab, it becomes kind of useless... In Classic calling %lsmagic produces (clipped for simplicity):

image

while in Lab, this is the output:

image

Lab defaults to showing JSON reprs when available, which typically makes sense, so while this is a change in behavior re. Classic, I'm inclined to think it's a good step forward.

Users can get the text via

image

but that's a pretty awkward experience. Do we really need lsmagic to produce JSON by default? It seems to me that generating plain text would be fine most of the time, perhaps with a --json/-j option for JSON output when desired?

@Carreau
Copy link
Member

Carreau commented Jun 19, 2019

That is used by the qtconsole to populate the qt menus dynamically I believe; so we would need to patch qtconsole to run with the --json option if we remove this default.

cc @ccordoba12

@ccordoba12
Copy link
Member

No problem in our side, the change should be quite simple to apply in qtconsole.

@DrAmygdala
Copy link

I'd like to work on this. If I understand correctly, we want %lsmagic to return plain text instead of JSON in Jupyter Lab like it does in the terminal, right?

That said, I'm not really sure where this might happen. I see that core/magics/basic.py defines MagicsDisplay._lsmagic which is called by MagicsDisplay.str but I'm not sure where and how this is changed for Jupyter Lab. Could anyone point me in the right direction?

@juhuebner
Copy link

This is quite some time ago but are there any news/ workarounds on this issue?

@fperez
Copy link
Member Author

fperez commented May 1, 2023

Sorry I missed your reply @DrAmygdala!! The needed change would be to make %lsmagic default to returning plain text, nothing needed in JupyterLab itself. As @ccordoba12 indicated, qtconsole could then adapt (we'd coordinate with them to ensure there's no breakage of their UX) to explicitly request JSON, with a (new, to be written) --json option. All these changes would be in IPython.

@DrAmygdala
Copy link

DrAmygdala commented Jul 23, 2023

Thanks for the clarification!

From what I understand, core.magics.basic.MagicsDisplay, which defines _lsmagic, has a _repr_json_ method. JupyterLab picks up on that and uses _repr_json_ instead of _lsmagic as is called from core.interactiveshell.InteractiveShell.run_line_magic, which is what gets called when you run %lsmagic from the IPython shell.

Assuming that JupyterLab shouldn't need to make a change, I think I'm missing something here. I assume there's something hiding the need for _repr_json_ from JupyterLab, but it's not clear to me what that set of layers is.

Perhaps I'm misunderstanding what is meant by default here. Assuming that default is what you get from the IPython shell, I believe it returns a string by default and it can be made to return a JSON by using something that recognizes the _repr_json_ method.

@fperez
Copy link
Member Author

fperez commented Jul 24, 2023

@DrAmygdala - it's a UX thing, nothing wrong with the underlying pieces of the implementation. But since a primary use case of lsmagic is to provide a human with info about magics, I think we should prioritize immediately human-readable outputs. That can be done simply by having a plain lsmagic call return str(output), while lsmagic --json can return plain output, which would be the currently returned objecd that has a _repr_json_.

@ccordoba12 indicated that adapting to this for the purposes of QtConsole wouldn't be a problem.

This is a widely used pattern in command-line tools, see for example how conda info does the same thing.

@ccordoba12
Copy link
Member

I assume there's something hiding the need for repr_json from JupyterLab, but it's not clear to me what that set of layers is.

Yep, I think that's what @fperez is suggesting: leaving the json repr of lsmagic to be under a flag and make it return its string repr instead by default.

In that case, I don't think we need to make any changes in Qtconsole because it can't display objects with json repr's.

@DrAmygdala
Copy link

I've seen the pattern as well, so I'm not lost there, I think. Perhaps what I'm missing then is the entry point.

It seems to me that, to use lsmagic (or any other magic), you register BasicMagics to MagicManager as is done in InteractiveShell.init_magics. Then, when you call it with something like InteractiveShell.run_line_magic you end up calling BasicMagics.lsmagic(*args, **kwargs) which returns an instance of MagicsDisplay. What you do with MagicsDisplay determines the output (eg. %magic applies str to it), and it doesn't seem to define a default for itself as far as I can tell. So far, that's my understanding.

What's not clear to me is where this default way of getting the %lsmagic output is. Where MagicsDisplay is being manipulated is unclear to me.

Stepping back from that line of thinking, the alternative I can see would be something like:

# BasicMagics
# ...
@magic_arguments.magic_arguments()
@magic_arguments.argument('-j', '--json', action="store_true", """Output as JSON""")
@line_magic
def lsmagic(self, parameter_s=''):
    args = magic_arguments.parse_argstring(self.lsmagic, parameter_s)
    md = MagicsDisplay(self.shell.magics_manager, ignore=[])
    if args.json:
        return md._repr_json_()
    return str(md)

@ccordoba12
Copy link
Member

Stepping back from that line of thinking, the alternative I can see would be something like

That looks good enough to me, but I couldn't tell for sure because I don't know the IPython code very well.

@s-kai273
Copy link

Hi!
I tried to implement the suggested code and created PR.

Hope someone to review and merge it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants