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

Limit number of completions returned #10743

Merged
merged 2 commits into from Aug 18, 2017

Conversation

Projects
None yet
3 participants
@takluyver
Copy link
Member

takluyver commented Aug 15, 2017

If there are a very large number of completions (e.g. tens of thousands of files), the notebook frontend can have trouble handling them - see jupyter/help#198 . I don't see any practical need for so many completions in the UI - if there are that many, you're likely to type some more letters and try completing again.

This limits the returned completions to 500. It's not configurable, but there's a clear constant so someone could patch it locally if there was an unexpected need for a larger number of completions.

@Carreau

This comment has been minimized.

Copy link
Member

Carreau commented Aug 15, 2017

JupyterLab May not like that as (IIRC) they use client-side completion filtering for as-you type. returning only the fist 500 may be having undesirable side effects.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Aug 16, 2017

Is it clear where it has difficulty? If it's in the display and not the message, maybe this filtering should be done client-side rather than kernel-side. If it's in the transfer/serialization of the message itself, then doing it in the kernel makes sense.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Aug 16, 2017

I didn't investigate that, but the issue was about 70k files appearing as completions. If it takes 15 characters for each completion, that's sending over 1MB of data. Even if the main slowdown is from display when it's local, if you're accessing a remote server over a low-bandwidth connection, that's a lot of data to be transferring when you hit tab.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Aug 18, 2017

Makes sense, thanks

@minrk minrk merged commit 03bb4e4 into ipython:master Aug 18, 2017

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +32.95% compared to 5319d5c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Aug 18, 2017

Do we want this backported to 5.x?

@Carreau

This comment has been minimized.

Copy link
Member

Carreau commented Aug 18, 2017

Do we want this backported to 5.x?

want ? Not really. Maybe we should for consistency though.

@takluyver takluyver added this to the 5.5 milestone Aug 18, 2017

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Aug 18, 2017

I'm inclined to think we should.

@meeseeksdev backport

@meeseeksdev

This comment has been minimized.

Copy link
Contributor

meeseeksdev bot commented Aug 18, 2017

There seem to be a conflict, please backport manually

Carreau added a commit to Carreau/ipython that referenced this pull request Sep 13, 2017

Backport PR 10743 on branch 5.x
Merge pull request ipython#10743 from takluyver/limit-no-completions

Limit number of completions returned
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment