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
Predictable and safe object inspection #13833
base: main
Are you sure you want to change the base?
Conversation
…ariant of the inspector Fixes ipython#13759 (or at least is the start of a fix for ipython#13759) This new file is from https://github.com/googlecolab/colabtools/blob/feb5dde3839251fd6fd8bf23e081d0a8b9a8187b/google/colab/_inspector.py
…ance info to the module
…es the info for pinfo, etc.
I see a difference in the %pinfo output between the two inspectors: class BadRepr:
def __repr__(self):
import time
time.sleep(10)
return 'a very long string'*100 Default inspector:
SafeInspector:
|
Colab special-cases tensorflow objects: if (
isinstance(shape, tuple)
or hasattr(shape, "__module__")
and isinstance(shape.__module__, str)
and "tensorflow." in shape.__module__
):
return "{} with shape {}".format(type_name, shape) Should the IPython implementation also specialcase tensorflow? If not, can we make it easy for a subclass to have its own special cases at this point? |
…ream code it is modifying for easier maintenance oinspect._render_signature is basically a copy of inspect.Signature.__str__ with a few tweaks. However, comparing the two functions is hard since it was basically rewritten for _render_signature. This commit makes _render_signature nearly identical to inspect.Signature.__str__ so it can be easily compared in the future, with clear notes about what was changed. There should be no behavior difference before and after this change.
…tdef With this change, the code between the two functions is largely identical, with the change that the signature default and annotation values are replaced with versions with safe repr functions. With this change, the SafeInspector._getdef now adopts the line-breaking behavior of Inspector._getdef.
This also introduces new getstr and getlen methods that can be overridden to have customized string and len methods. For example, for a safe inspector, these may avoid calling the object methods in case it is not safe.
…ormation This was added in Colab's implementation, and could be used to highlight the lines of the definition of an object, for example.
…so no need to catch them again
There should be no change in functionality from this commit
There should be no change in functionality from this commit
- simplify many of the methods - align implementation of info() more closely with upstream oinspect info()
…ting getstr and getlen By now, the oinspect info() implementation and the safe inspect info() implementation are basically aligned, with the upstream implementation calling out to getstr and getlen when it is formatting an object for display.
I tried to unify the safe inspect info() and oinspect info() implementations. They mostly align. There have been many updates to oinspect info(), which I tried to reason about in the context of the safe inspection. There are still a few notes about TODO items I'm not sure about, but by in large this brings safe inspection in line with what the oinspect info currently produces. It seems that there are only a few key places where we needed to change the upstream implementation, so I put stubs in for functions the safe implementation can override. Then deleted the safe_inspect info() code. This greatly simplifies things. I spent a lot of time reading the Python inspect module code to convince myself that it was safe to rely on standard library code for more things. I tried to clearly indicate reasons behind many of the changes in the commits in this PR. |
This is tested in test_oinspect.py:test_info_awkward, for example.
0bad685
to
42a2913
Compare
…ure function to not call an object's repr method.
…not call repr() or str() where possible
I tried running all of our existing oinspect tests with the SafeInspector inspector, along with one other test: class BadRepr:
"""
A honeypot repr that should not be called
"""
def __repr__(self):
import time
breakpoint()
time.sleep(10)
raise ValueError("DON'T CALL ME")
return 'A VERY BAD REPR'
def test_info_badrepr():
inspector.info(BadRepr()) and we have a problem: the Python inspect module calls the repr of an object in many different places for error messages. The original Colab code sort of gets around this by trying to do the following before calling inspect's signature method:
So which approach should we use?
To experiment, I made the copy of parts of inspect.py and updated it to not call |
Fix the test to be passing as expected
…fe_signature.py We don't want to reformat oinspect_safe_signature.py so that it is easy to diff against upstream inspect.py
Update: after thinking about this for a bit, I think it's untenable to carry a copy of the Python inspect.py file. I'm looking into how to back that part out and make the safe inspect lighter weight. Another option is to go upstream to Python and ask if they can not print the repr of the object in error messages. Then we can just mostly use upstream inspect. |
In #13759, we discussed how Colab's inspection routine guards against calling an arbitrary object's
__repr__()
method, since in practice that can take a lot of time and/or memory, depending on how the object was implemented.This PR experiments with including an updated/modified version of the Colab implementation (licensed Apache 2.0), and making it easy to configure the interactive shell's inspector implementation.
Use the new inspector with
CC @craigcitro