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

Bring back readline capability? #10364

Closed
ivanov opened this issue Mar 1, 2017 · 38 comments
Closed

Bring back readline capability? #10364

ivanov opened this issue Mar 1, 2017 · 38 comments
Milestone

Comments

@ivanov
Copy link
Member

ivanov commented Mar 1, 2017

Even if it's not what is used by default, I've experienced and heard about enough other people experiencing pains with the prompt_toolkit-based input breaking people's workflows, that I think we should seriously consider bringing that code back in.

I think it was a mistake to remove it altogether, and change the default at the same time.

@ivanov
Copy link
Member Author

ivanov commented Mar 1, 2017

I'm willing to do the work to get this back in time for 6.0, so I'll tag #10329 here.

@ivanov
Copy link
Member Author

ivanov commented Mar 1, 2017

I think this is a particularly important issue because long time users and advocates of IPython are finding themselves frustrated by or even unwilling to use IPython 5 as it is right now.

@Carreau
Copy link
Member

Carreau commented Mar 1, 2017

I'm happy to add that back as an option, though most of the code has been removed and that will likely be a lot of efforts to reintegrate readline.

@Carreau Carreau added this to the 6.0 milestone Mar 1, 2017
@Carreau
Copy link
Member

Carreau commented Mar 1, 2017

If we can figure out how to make that optional (so that it can evolve faster than than core to re-fix potential bug that were introduced by the revival, that would be great.

@Carreau
Copy link
Member

Carreau commented Mar 1, 2017

We might want to delay #10356 if we bring RL back.

@Carreau
Copy link
Member

Carreau commented Mar 1, 2017

See #9260 as well.

@Carreau
Copy link
Member

Carreau commented Mar 1, 2017

And #9399 is the bulk of the removal.

@takluyver
Copy link
Member

I am strongly -1 on this. We were able to drop a lot of code and complexity when we ditched readline, (and we're working on dropping still more), while dramatically improving the user experience for most users. If we bring back a readline option, we have all the complexity of both interfaces to maintain ~forever. We always knew a change this big would break some workflows, but I'm confident that it's a net improvement, and I do not want us to be in the business of maintaining two alternative terminal interfaces.

I would rather we:

  1. Continue to work on anything we can improve both in IPython and in prompt_toolkit to remove obstacles to switching. Jonathan has generally been impressively responsive and helpful whenever we've pinged him about issues relating to PTK, so I think we have a good chance of getting any changes we need accepted. This includes things like better documenting how to set custom shortcuts, and maybe experimenting with reading .inputrc (though I'd rather that was an off-by-default option, for reasons I've explained elsewhere). If we give people an easy option to go back to readline, these improvements are not going to happen.
  2. If there are still people who really insist on using readline, spin out a separate package like rlipython which provides the terminal interface, but make it very clear that it's community maintained, not something we are officially supporting.

@pfmoore
Copy link
Contributor

pfmoore commented Mar 2, 2017

Given that there were always a number of problems with pyreadline on Windows (not least of it, the fact that it globally affected the standard Python REPL as well as IPython) I'd prefer that not requiring (py)readline remain the default, at least on Windows. If a user prefers to install pyreadline manually and enable it, that's fine, but IPython should work without pyreadline present, and not include it as a dependency of a normal install.

@Carreau
Copy link
Member

Carreau commented Mar 2, 2017

If there are still people who really insist on using readline, spin out a separate package like rlipython which provides the terminal interface, but make it very clear that it's community maintained, not something we are officially supporting.

I think that is reasonable, though I believe have some integration (like a flag and/or an env variable) with IPython that restore previous behavior is reasonable. The goal would be to have already well spread scripts and recipes that do rely on IPython readline still working with a simple switch.

My guess is that a simple config in IPythonApp, that make the TerminalInteractiveShell class use a traitlet, plus a --readline flag that have a preset value to a known external package would be enough.

The ipython as a separate package will also allow to not block IPython 6, and get a fast iteration cycle if necessary.

If we give people an easy option to go back to readline, these improvements are not going to happen.

I doubt that is completely true. The multiline, highlighting, and now completion are still a large advantage of the PTK interface. And user that are pinning to 4.x because they can't even use bothe RL/PTK in parallel have no incentive send patch to IPython / PTK to smooth the behavior.

@takluyver
Copy link
Member

I wouldn't integrate it with a switch, because that makes it look like a supported option, and people will file bugs about it here. rlipython is less typing than ipython --readline anyway, and if people are keen to use it as ipython, they can alias it.

@rgbkrk
Copy link
Member

rgbkrk commented Mar 2, 2017

Just a note on the current experience of ctrl-r:

In Bash, when you hit ctrl-r then hit escape, it will exit you out.

In this one it does nothing when it should close the search area.

@Carreau
Copy link
Member

Carreau commented Mar 2, 2017

Just a note on the current experience of ctrl-r:
In Bash, when you hit ctrl-r then hit escape, it will exit you out.
In this one it does nothing when it should close the search area.

This one is easy to fix, but not totally:

 ~/dev/ipython master [1]"!!" $ git diff
diff --git a/IPython/terminal/shortcuts.py b/IPython/terminal/shortcuts.py
index 22ad111..89713cd 100644
--- a/IPython/terminal/shortcuts.py
+++ b/IPython/terminal/shortcuts.py
@@ -54,6 +54,10 @@ def register_ipython_shortcuts(registry, shell):
     registry.add_binding(Keys.ControlC, filter=HasFocus(DEFAULT_BUFFER)
                         )(reset_buffer)

+
+    registry.add_binding(Keys.Escape, filter=HasFocus(SEARCH_BUFFER)
+                        )(reset_search_buffer)
+
     registry.add_binding(Keys.ControlC, filter=HasFocus(SEARCH_BUFFER)
                         )(reset_search_buffer)

That will work, but the I-search-backward: will still be visible until you press a new key. This new key will behave has if the backward search was dismissed, but it feels extremely weird.

@fperez
Copy link
Member

fperez commented Mar 4, 2017

Thanks for opening this, @ivanov! I'm going to go over the details a bit more carefully, I'm also going to encourage more feedback on the list on this. It's evidently a tricky balance and contentious, thus a good sign that more voices can at least inform our decision process.

@ivanov
Copy link
Member Author

ivanov commented Mar 4, 2017

Just an update on the possible way forward for this effort: instead of bringing the removed code into IPython proper, @Carreau made a new project with just the old readline-based code over at Carreau/rlipython, and made #10373 which allows users to customize this piece.

@jonathanslenders
Copy link
Contributor

FYI: the issue that is described in the mailing list:

I ask mainly because in SMC at least, if you try to paste in multiple
lines it pastes in the first line and silently ignores everything
else
. This may be an issue with the level of xterm support in SMC
(i.e., term.js). However, it's pretty frustrating.

This indicates that this terminal does not support bracketed paste.
IMHO: I think that these days we can expect from any terminal to support it: https://cirw.in/blog/bracketed-paste (At least prompt_toolkit will require bracketed paste, for pasting multiline text.)

It could be worth adding readline support again, but notice that multiple of the mentioned bugs are already fixed or will receive a fix. Biggest problems are probably the Emacs terminal (which is not really an xterm-compatible terminal and will never be supported), and .inputrc support which will eventually come, but due to bandwidth/priority issues on my side, will take some time.

In any case, keep reporting user experience issues. This is important.

@minrk
Copy link
Member

minrk commented Mar 5, 2017

@pfmoore that's a good point. I would doubt any Windows environments are hurting from the lack of pyreadline. If we bring back readline UI, it probably makes sense to do this for 'true' readline only, whether it's in IPython or the rlipython package.

One advantage of the separate rlipython package is that it can be used with the existing IPython 5 release, whereas doing it in IPython itself would obviously only work for new IPython releases.

@matthew-brett
Copy link
Contributor

I'd like to add #9799 as another source of discomfort with the prompt_toolkit defaults. I'd be perfectly happy with an extra external package to install.

@stonebig
Copy link
Contributor

stonebig commented Mar 7, 2017

pyreadline is a pain on Windows, but prompt-toolkit has also issues. So between 2 unsatisfactory solutions, the less "technical debt" solution looks like the best solution: prompt-toolkit.

@matthew-brett
Copy link
Contributor

I think the point is that prompt_toolkit is better for some people, and readline is better for other people. So the question is, is it necessary to cut the readline people loose to avoid maintaining the extra code?

@Carreau
Copy link
Member

Carreau commented Mar 8, 2017

pyreadline is a pain on Windows, but prompt-toolkit has also issues. So between 2 unsatisfactory solutions, the less "technical debt" solution looks like the best solution: prompt-toolkit.

I think the point is that prompt_toolkit is better for some people, and readline is better for other people. So the question is, is it necessary to cut the readline people loose to avoid maintaining the extra code?

Note that right now the attached PR (#10373) is 4 lines, and is just a configuration option to make it a configuration option. The readline code would not even be in IPython and would be an extension.

The real question is:

  • A: Is it ok if readline-ipython is a separate executable with a different spelling.
  • B: Or for script and extensions that shell-out to IPython, (emacs for example), should it be an IPython configuration option.

In both case readline will not be made a dependency of IPython again.

It does influence a couple of functionalities that are simpler to keep in IPython, but the main contention right now is A or B.

@tacaswell
Copy link
Contributor

From the emacs side either is fine. B seems more flexible in the long run.

@matthew-brett
Copy link
Contributor

I guess, for those of us who prefer readline, we'd want readline for IPython shelling out as well - so B.

@fperez
Copy link
Member

fperez commented Mar 21, 2017

I've been thinking about this one for a while and talking to a few folks on the team I've had a chance to engage in-person. Here's my take on the matter. I'm not yet calling a final decision, but I am trying to move us closer to a resolution. BTW, I want to emphasize that I am very grateful to the Prompt Toolkit team, as it's given us in the vast majority of cases an amazing, modern and high-quality user experience. On to the matter at hand:

  • the concerns of users who need readline are very real, and I think we should find a good solution for them now, even if the cost is carrying some extra code in/around IPython.

  • We'll continue working with the PT team to improve it towards feature parity with readline where that's an issue (I'm aware it has a vast collection of features RL doesn't have).

  • Even if PT improves further, I can still see situations where plain old readline will be valuable. PT, by virtue of its sophistication, is likely to be slower or more brittle when say running inside tmux over ssh within an ipython shell inside Emacs. That is a valid use case that we should support well, and that we used to.

Given this, I think a good compromise solution is along the lines of what @Carreau has proposed in his PR:

  • we support readline but only via a third-party package that has to be manually installed by the user.

  • the user can then either set the use of this package in their config file, call it at the command line, or write a shell alias or tiny wrapper script. Yes, this requires a bit of extra work on the part of users of this corner case, but it gives them a clean solution at minimal cost, while the majority of users who can use PT (and benefit from its features) continue doing so uninterrupted.

This does mean supporting for a while this extra package, but I don't imagine it will be that much work: that code barely changed in quite a while, and we're not talking about adding lots of new features. Simply maintaining compatibility with existing, historical baseline behavior.

The one cost that I see as somewhat significant is that (mentioned by @Carreau) there may be code we'd like to rip out that this would prevent us from removing. I see that as a price we should pay for the benefit of our users, at least for now. If at some future time we truly find ourselves in a situation where PT is a 100% replacement for RL in every conceivable case, we can revisit. But for now, keeping a bit of slightly stale, special-purpose code for the benefit of some subset of our users is the right thing to do. Over the years we've had multiple kinds of special-case code (windows, py2/3, etc.) and I'm sure we will again in the future. Serving the workflows of our users should, I think, take priority over a code cleanup (in this situation, not making a blanket statement).

How does this sound?

@fperez
Copy link
Member

fperez commented Mar 21, 2017

BTW, I think this 'fix' ought to be backported to the 5.x series: if anything, I suspect the population of users still on python 2.x has a good overlap with that of people working remotely on older servers. So my vote would be for making the external package be python 2-3 compatible, and to backport the ipython side of the code to 5.x.

@Carreau
Copy link
Member

Carreau commented Mar 22, 2017

Thanks Fernando for taking the time to respond, and especially for reaching to several people.
I'll work on polishing my PR, merge it and backport it to 5.0.

I've rlipython on GitHub for anyone who is interested in maintaining the readline interface. I won't maintain, and won't publish it on PyPI, but you are welcome to do it, and it might be a good things to move this to https://github.com/ipython-contrib if you need an org.

Thanks !

@fperez
Copy link
Member

fperez commented Mar 22, 2017

I actually think we should host this on the main IPython org, and put it on pypi. We can also send a slightly more welcoming message to the community, along the lines of:

this is something that we offer in support of historical compatibility and certain specific use cases where our main interface (prompt-toolkit) isn't optimal. But we do not envision any significant development beyond fixing critical bugs. We only have the resources to offer this as a best-effort solution. If you are interested in doing any significant development on this tool, please let us know via an issue, and we can explore transferring maintainership of the package to you."

If @ivanov has a few cycles to put in this direction that would be great, but if not I'm willing to do it over the next couple of weeks. I think it's an important part of engaging with all corners of our user community.

Thanks @Carreau for pushing the work this far!

@ivanov
Copy link
Member Author

ivanov commented Apr 10, 2017

Thanks everyone, especially @Carreau for getting rlipython up. I've started working on it and whipping it into shape. In case anyone missed it, it now lives under ipython/rlipython. I'll try to get a release out in the next two weeks as I start working with it, but for now I can say it works! There's one known issue right now - In/Out prompts not being colored, which I am tracking in ipython/rlipython#3, but other than that, it seems great.

@Carreau Carreau modified the milestones: 5.4, 6.0 Apr 18, 2017
@fperez
Copy link
Member

fperez commented Apr 21, 2017

@ivanov, ping me on that if you'd like a hand... I'd offered above but I was more than a bit naïve about my time availability these last few weeks. Nonetheless I want to help this reach a solid solution for all cmd line users, so I'm still willing to pitch in if you need a hand (and would enjoy doing so :)

@Carreau
Copy link
Member

Carreau commented May 10, 2017

Closed by #10373 and backported as #10463

@Carreau Carreau closed this as completed May 10, 2017
@heroxbd
Copy link

heroxbd commented May 11, 2017

Hi, with this issue closed, is there any way to disable prompt_toolkit completely?

@Carreau
Copy link
Member

Carreau commented May 11, 2017

Hi, with this issue closed, is there any way to disable prompt_toolkit completely?

yes, if you set the TerminalIPythonApp.interactive_shell_class to rlipython in your config file, then prompt_toolkit should not even be imported. The readme of rlipython give more info on how to do this.

@ivanov
Copy link
Member Author

ivanov commented May 17, 2017

For anyone still subscribed on this issue: I just released rlipython version 0.1.0, you can now pip install rlipython and get the old readline working by default in IPython after you import rlipython; rlipython.install().

@dimpase
Copy link

dimpase commented Oct 8, 2017

Late to the party, but let me reiterate that RT at present is a no-no is to projects that have modules not designed for multithreaded initialisation, see Sage's ticket 22766. (And this is the primary reason we need to do something about it in Sage).

As well, a comment on a general problem with the present RT: it triggers multi-threaded importing of Python modules while doing tab-completion. We very much doubt that it is safe.

Last but not the least - given that IPython tab completion causes segfaults, how about a means for non-interactive testing of it?

@takluyver
Copy link
Member

@jonathanslenders have you run across any issues with completion running in a separate thread? Is there an option to make it run synchronously?

@jonathanslenders
Copy link
Contributor

Hi @takluyver,

Excuse me for the late reply. I got married last month, and so I've been offline for a while. ;)

I'm working on prompt_toolkit 2.0 which already supports both synchronous (in the main thread) and asynchronous (in other thread) completion. I've been working on 2.0 for almost a year and hope to release that in a couple of months.

For synchronous completion, you have to keep in mind that if the completer is not super fast, that this will introduce a delay after every key stroke (if you have complete-while-typing on). I believe that Jedi for instance is not super fast.

I know that certain libraries have some issues with asynchronous completion. I think we should encourage the authors of these libraries to make sure that importing them in the first place can be done from any thread. In the future, I'd make it an option to run the completions sync or async, but still prefer to have async completion by default. It makes the user experience so much nicer.

@dimpase
Copy link

dimpase commented Oct 21, 2017 via email

@takluyver
Copy link
Member

Congratulations! Of course there's no need to apologise - we get your awesome library for free, so any support has to be as and when it's convenient for you. Best wishes to you and your partner.

I think we'll likely switch to synchronous completions when they're available. We don't use complete-as-you-type, because we enable the history search feature which is mutually exclusive with that.

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