Fix tab completion#2055
Conversation
| dev = Device(0) | ||
| dev.set_current() | ||
| mr = DeviceMemoryResource(dev) | ||
| assert not mr.is_ipc_enabled, "test setup: mr should not be IPC-enabled" |
There was a problem hiding this comment.
This is only testing one specific class that we know broke things. If we accept this PR, we should probably add testing that /all/ of our Cython classes autocomplete correctly.
|
|
Test failures indicate something wrong in the design of the test -- not a problem with the monkey-patch itself. |
| if not (hasattr(sys, "ps1") or sys.flags.inspect): | ||
| # Plain `python script.py`, `python -c ...`, pytest, etc. | ||
| return |
There was a problem hiding this comment.
This doesn't seem quite correct, where if I just run python from a shell it drops me to a REPL that I should be able to get autocompletion with but sys.flags.inspect is 0.
There was a problem hiding this comment.
Yes, but ps1 is there, right. It WFM with python.
| # This works by overriding the `property` built-in with a custom subclass of | ||
| # property, but only in the rlcompleter module. This subclass overrides the | ||
| # `__instancecheck__` method to also return True for getset_descriptor and | ||
| # member_descriptor types, which are what Cython uses for properties on cdef | ||
| # classes. |
There was a problem hiding this comment.
Are we concerned about any implications of other modules Cython classes getset_descriptor objects?
2 participants
This is a possible fix for #2053.
I almost never reach for monkey-patching as a solution, however in this case:
The alternatives are way worse and intrusive and force us into API design corners for the niche use case of tab completion (see my comments about the agent's other suggestions).
The monkeypatch is quite narrow -- it doesn't patch builtins that might have broad implications. It should only affect autocompletion with
readline. A project like that might exist, but it's probably pretty niche. IPython and Jupyter are not affected by the bug to begin with, and the patch has no effect on them. (The patch is /installed/ with IPython, but then the code never gets called. I experimented with detecting IPython and not installing the patch, but it was complex, specific to IPython, and ultimately doesn't matter).This monkeypatch is idempotent. If it gets cargo-culted to multiple projects, and those projects get imported in the same process, it will still work and be effective.
(Pending the CI run passing) It should be fine across all versions of Python we support, and there is a test here to confirm that going forward. If Python "fixes" the issue such that this patch is no longer needed, the test here should catch that and we can gate it at that point (as part of the usual "bring up on a new Python version" process).