-
Notifications
You must be signed in to change notification settings - Fork 128
Noticeable delay in prompt return #1503
-
I had noticed a delay in getting prompt back even if all i did was just repeatedly press enter in the shell. The effect was somewhat significant if my command used the read_input function.
So i looked up the flame graph to identify the bottleneck. Seems _run_cmdfinalization_hooks does a subprocess call that's taking some amount of time in the chart:
def _run_cmdfinalization_hooks(self, stop: bool, statement: Optional[Statement]) -> bool:
"""Run the command finalization hooks"""
with self.sigint_protection:
if not sys.platform.startswith('win') and self.stdin.isatty():
# Before the next command runs, fix any terminal problems like those
# caused by certain binary characters having been printed to it.
import subprocess
proc = subprocess.Popen(['stty', 'sane'])
proc.communicate()
When I commented this out locally, the delay disappeared and everything still worked fine.
ChatGPT had suggested replacing the subprocess with termios like this(Not sure how that works as i couldn't see any differences with/without the subprocess code so i am unsure how to test this out):
import termios, sys
def reset_tty():
if sys.stdin.isatty():
fd = sys.stdin.fileno()
# sane mode roughly equals this combo:
attrs = termios.tcgetattr(fd)
attrs[3] |= (termios.ECHO | termios.ICANON | termios.ISIG)
termios.tcsetattr(fd, termios.TCSANOW, attrs)
Can you please check on your side & let me know; The issue is easily seen if you try doing everything quick on the shell.
Beta Was this translation helpful? Give feedback.
All reactions
@Sripadvallabh We merged in the PR that made similar changes to what you suggested. Changes will be in the 3.0.0 release.
Replies: 6 comments 7 replies
-
@Sripadvallabh Can you please give us a little bit more info to help us reproduce. Key things of interest:
- Version of cmd2
- Python version
- Operating system
- Terminal program used
Code for a toy cmd2 application that demonstrates this would also be helpful.
Beta Was this translation helpful? Give feedback.
All reactions
-
I did some initial testing on a system with the following setup:
- Latest cmd2 from
mainbranch on GitHub - Python 3.14.0rc2
- macOS Sequoia 15.6.1 on an older Intel core i9 Macbook pro
- iterm2 3.5.14
In this testing I modified cmd's built-in timing measurement to capture the full command lifecycle timing. With the code as-is, the full lifecycle for an empty command on this older computer was about 0.007 seconds (7 thousands of a second). This is not something I would even remotely call "slow" given typical human reaction times.
I did experiment replacing the subprocess call with a more optimized version of the termios.tcsetattr code you showed above where I cached the initial settings as an instance member within the cmd2.Cmd class to prevent needing to call tcgetattr again. This did substantially speed things up to the point where an empty command ran with an elapsed time of about 0.00011 seconds (about 64 times faster), but this should not really be noticeable one way or the other.
@Sripadvallabh I trust that your personal observations of slowness are valid. But I believe there is some important difference or differences in the way we are testing, so I think I need more info from you to try to reproduce.
Beta Was this translation helpful? Give feedback.
All reactions
-
I created draft PR #1504 to experiment with changes suggested in this Discussion.
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh We merged in the PR that made similar changes to what you suggested. Changes will be in the 3.0.0 release.
Beta Was this translation helpful? Give feedback.
All reactions
-
Thanks @tleonhardt, let me try with the latest & check.
Below is the information you had requested:
cmd2 version: cmd2 2.4.3
Python version: Python 3.6.8
I am using linux:
$ cat /etc/os-release
NAME="Red Hat Enterprise Linux"
VERSION="8.8 (Ootpa)"
ID="rhel"
ID_LIKE="fedora"
VERSION_ID="8.8"
PLATFORM_ID="platform:el8"
PRETTY_NAME="Red Hat Enterprise Linux 8.8 (Ootpa)"
$ lscpu | head -n 5
Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Terminal program: iterm2 only
Due to some confidentiality agreements, unable to disclose exact information on the cmd2 app, but it is initialized like this:
# cmd2 initialization
super().__init__(
persistent_history_file=settings.get_system_default('persistent_history_file'),
persistent_history_length=settings.get_system_default('persistent_history_length'),
startup_script=settings.get_system_default('shell_startup_script'),
include_py=True,
allow_redirection=True,
terminators=[],
shortcuts=settings.get_system_default('shell_shortcuts')
)
# Set necessary cmd2 instance variables
self.self_in_py = True
self.max_completion_items = settings.get_system_default('shell_max_completion_items')
self.hidden_commands.append('quit')
self.remove_settable('allow_style')
self.remove_settable('always_show_hint')
self.remove_settable('echo')
self.remove_settable('editor')
self.remove_settable('feedback_to_output')
self.remove_settable('max_completion_items')
self.remove_settable('quiet')
self.remove_settable('timing')
I use read_input a lot, so might be related with that as well especially when the command quits after using it:
def do_xxx(self, args):
input = self.read_input('Input:')
return
I would face noticeable delays when using commands like 'xxx' which uses read_input.
Please let me know if you need any more information.
Sorry about the confidentiality part, but i can guarantee it's not for long. Once the app gets open-sourced after development, i will definitely share it with you guys!
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh It's always fascinating for me to see the myriad ways in which people use cmd2 which end up being so very different than I ever envisioned!
Personally, I've never used self.read_input() within a command. I always use the @cmd2.with_argparser decorator to define command arguments using argparse that can get nicely validated when the command is called so that all required input for a command is provided up front.
I think @kmvanbrunt might have more experience using read_input() within a command and might be able to more intelligently comment about why that might impact performance.
I noticed you are using a version of Python which hit end-of-life (EOL) several years ago and similarly outdated version of cmd2. I have no idea what your constraints are, but if you could upgrade to a newer version of Python and cmd2, it would likely be a good thing. Unfortunately, the version of cmd2 you are using is the last one that works with Python 3.6.
Beta Was this translation helpful? Give feedback.
All reactions
-
Actually, @tleonhardt my use-case was a pretty different one. I needed a prompt within a command to get multiple inputs from user & need to return back a response for each of them since the command was to be designed that way as each inputs get tied together to the previous input so a chain of inputs was necessary. I asked users to use '+' within the cmd2 command itself to separate them, but we missed the response for each of the individual input. Hence, the current design. I also needed autocompletion & argparse functionality so i opted to use the easy read_input provided by cmd2. It is far better than prompt_toolkit or other packages as it's easy to use, no need to do another extra import & only needs a setup with the Cmd2ArgumentParser().
Sorry, i noticed i put a wrong version of python above. My actual python version is: Python 3.8.2.
Thanks for addressing the issue posted. Actually i am using cmd2 2.5.4 & i have applied a patch for now to make it work. I am yet to migrate to 3.0.0.
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh That sounds like a very interesting an atypical use case. I'm glad cmd2 is able to meet your needs.
Since you are using Python 3.8, you should be able to painlessly upgrade to cmd2 2.5.11.
We haven't released 3.0.0 yet, but hope to do so within about a month. But that will required Python 3.10 or newer.
Beta Was this translation helpful? Give feedback.
All reactions
-
@tleonhardt, it seems we can't move to 3.0.0 as the version of python we have currently is Python 3.8.2. Could you please backport the fix to the cmd2 latest that supports this python version please?
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh Python 3.8 hit EOL about a year ago and 3.9 is going to hit EOL in a couple weeks: https://devguide.python.org/versions/
As such, I would strongly encourage you to upgrade the version of Python you are using if at all possible to one receiving security updates.
If you need support for Python 3.9 we could probably backport the fix to the 2.x branch which is currently at 2.7.0. However, we dropped support for Python 3.8 starting with 2.6.0, so it is no longer possible for us to backport a fix for Python 3.8.
Beta Was this translation helpful? Give feedback.
All reactions
-
I just checked with the python versioning backend & found it is now recommending to use 3.10.10. So, the equivalent cmd2 version pip uses is the latest 2.7.0. This doesn't have the fix i am not wrong. We can wait for 3.0.0, but since we are launching the new shell app for external users next month for the first time, we might need the fix along with it. Pardon me for this question, but may i know if 3.0.0 is possible to get by october 1st week?
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh
We have a beta version out currently, version 3.0.0b2, and plan to put out a 3.0.0rc1 release candidate within the next day or so. These are releases we expect to be stable and equivalent to the final release.
If you wanted to test one of them out, we'd appreciate any feedback you might have. The only difference between the beta and upcoming release candidate is we made some minor documentation improvements for the release candidate.
Beta Was this translation helpful? Give feedback.
All reactions
-
@Sripadvallabh We just put out the 3.0.0rc1 release candidate. We don't have any changes planned for the official 3.0.0 release, but we do want to wait a week or two for user feedback based on the release candidate.
I'd recommend using the 3.0.0rc1 release candidate for your release. We plan to get the official 3.0.0 release out sometime in mid-October, but we won't have the official release out by Oct 1.
Beta Was this translation helpful? Give feedback.