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

Fix richInspectVariables #943

Merged
merged 4 commits into from
Jun 3, 2022

Conversation

davidbrochart
Copy link
Collaborator

It seems that we can always use the interpreter to get the rich representation of the variable, whether we hit a breakpoint or not. The problem with the current situation where we hit a breakpoint and display an HTML representation of a variable is that the HTML is striped and we get something like 'text/html': '<div>\n<style scoped>...le>\n</div>' instead of the full HTML. I can't find where it is striped, but if we don't need this branch anyway, can we just get rid of it?
@JohanMabille what do you think?
Also, @fcollonval might have some feedback since he refactored that part.

@fcollonval
Copy link
Contributor

Does this mean the strip is related to the evaluate debug request?

@davidbrochart
Copy link
Collaborator Author

davidbrochart commented May 30, 2022

Maybe, I can't find where it is striped. It is received already striped from the ZMQStream, but the evaluate debug request makes ipython generate a valid HTML. So it's somewhere before being sent on the ZMQ channel.

@davidbrochart
Copy link
Collaborator Author

I created a test which shows that the text/plain repr is striped.

@davidbrochart
Copy link
Collaborator Author

The issue was that SafeRepr is used to get a representation of an evaluation, which has some class attributes to strip the representation. Setting these values to a high number disables the stripping, although it is not very clean.
The new test in #944 passes with these changes.
Thanks to @fcollonval for spotting the issue. Do you think this fix is acceptable?

@davidbrochart
Copy link
Collaborator Author

davidbrochart commented Jun 2, 2022

To be more precise, we set maxstring_inner to 2 ** 16 (like maxstring_outer) and maxother_inner to 2 ** 16 (like maxother_outer). These values were set to 30 previously, which means that the maximum length of a representation was limited to 30 characters, having a head of 30 * 2 / 3 = 20 and a tail of 30 / 3 = 10, and ... in between to show that the repr was truncated. But in the debugger we don't want the representation to be truncated, especially if it's an HTML repr, since it would lead to invalid HTML.
Since these values are class attributes, it works for us but it could also impact every other user of debugpy, if it were used in the kernel, although it's not very likely. A cleaner solution would be to provide a way to configure this SafeRepr in debugpy/pydevd, but I don't know if it's (easily) feasible, any thought @fabioz?

@fcollonval
Copy link
Contributor

Thanks @davidbrochart for digging that up. I think a cleaner solution is to set SafeRepr.raw_value = True as other limiters exists too (like on collections):

https://github.com/microsoft/debugpy/blob/5e80ce1f774d84878bbd97a879ec4e381e948526/src/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_safe_repr.py#L268

To avoid side-effect for other use cases, could we apply the change in a transient fashion during the evaluation request?

@davidbrochart
Copy link
Collaborator Author

Thanks for helping @fcollonval!
Actually setting SafeRepr.raw_value = True doesn't work because it doesn't escape \, and so it breaks the evaluation.
But I found a clean way to set the string length limits to a high value just for our request.

@fabioz
Copy link

fabioz commented Jun 2, 2022

@davidbrochart what's the use case here? Can you give me the bigger picture? I'm not that fond of raising the limits for all requests because it can slow down debugging on some cases (i.e.: if the user has a 1MB string having that serialized all the time isn't ideal).

I saw you linked code using the clipboard context to request the data. If you can use that for your use case that's probably a better solution.

@davidbrochart
Copy link
Collaborator Author

Thanks @fabioz, we want to send an evaluate request with some code that computes the representation of a variable in IPython. This representation can be e.g. a big HTML string, and it should not be truncated because we show it in JupyterLab.
I'm wondering if it's better to send "context": "clipboard" or "format": {"rawString": True}. @fcollonval mentioned that with the former, other limits might apply on e.g. collections, but with the latter we still need to escape \ in the reply.

@fabioz
Copy link

fabioz commented Jun 2, 2022

@davidbrochart well, "context": "clipboard" should provide it as a repr(obj) without limits, whereas rawString will get the string directly (without limits too and without doing the repr -- or if dealing with bytes it'll decode it using latin1).

I don't really know what's best for the use-case you have (it depends on how you're going to use it afterwards...).

@davidbrochart
Copy link
Collaborator Author

davidbrochart commented Jun 2, 2022

I'm not sure, I think we can get back bytes if the type is e.g. image/png, so I would have thought that we could use rawString and do the repr ourselves, since we eval after that:

eval(repr(b"123"))
# b'123'

eval(repr("\n"))
# '\n'

But if bytes are decoded using latin1 that's not going to work, right?

@davidbrochart
Copy link
Collaborator Author

davidbrochart commented Jun 2, 2022

Anyways, I've tried it in JupyterLab with an image and it works fine with "context": "clipboard", so let's merge this.
Thanks @fabioz!

@blink1073
Copy link
Contributor

I tried kicking the two failing builds, but they still failed

@davidbrochart
Copy link
Collaborator Author

The last commit reverted the changes, we have the same failures so I guess it's not related. I'm going to re-apply them.

@blink1073
Copy link
Contributor

I just kicked https://github.com/ipython/ipykernel/actions/runs/2412265322 and it passed

@davidbrochart
Copy link
Collaborator Author

Yeah, it seems to be a bit random, but 448e16b is identical to main and it failed.

@davidbrochart
Copy link
Collaborator Author

I re-launched the only failing build and it passed.

@blink1073
Copy link
Contributor

Great, thanks!

@blink1073 blink1073 merged commit 82179ef into ipython:main Jun 3, 2022
@davidbrochart davidbrochart deleted the fix_richInspectVariables branch June 3, 2022 20:10
@blink1073
Copy link
Contributor

I can cut a release on Monday

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

Successfully merging this pull request may close these issues.

4 participants