Reintegrate jedi #10182

Merged
merged 4 commits into from Feb 3, 2017

Conversation

Projects
None yet
4 participants
@Carreau
Member

Carreau commented Jan 19, 2017

Not requesting review, just committing before going home, and indicating that I'm starting to work on that (help welcomed, feel free to push on the branch).

This try to integrate the Jedi (0.9+) completion machinery into IPython (prompt toolkit interface only for now, cause PTK is the best)

Right now this is (and will be) mostly through private API which will return jedi completion alongside IPython completions that will help me to set up a debugging that compare both and pop-up a warning if IPython found a completion that jedi missed.

For the little testing I did this fill fix/implement many of our tab-completion labeled issues.

@Carreau Carreau added this to the 6.0 milestone Jan 19, 2017

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 19, 2017

Member

First step in reintegrating Jedi

If Jedi is installed expose a private API use it with prompt toolkit.
Jedi does not yet provide all the completion IPython has, so this is
still a bit awkward. In order to debug this (and see what is Jedi
provided we for now inject a fake Jedi/IPython delimiter in the menu.

Next step would to tag completions we don't expect Jedi to have and not
mark them as "missing".

Jedi completion and this behavior are enabled by default, but could
likely be opt-in.


Here you can see that jedi does not propose .cache as a completion as Jedi does not complete path.

screen shot 2017-01-18 at 21 38 20

Member

Carreau commented Jan 19, 2017

First step in reintegrating Jedi

If Jedi is installed expose a private API use it with prompt toolkit.
Jedi does not yet provide all the completion IPython has, so this is
still a bit awkward. In order to debug this (and see what is Jedi
provided we for now inject a fake Jedi/IPython delimiter in the menu.

Next step would to tag completions we don't expect Jedi to have and not
mark them as "missing".

Jedi completion and this behavior are enabled by default, but could
likely be opt-in.


Here you can see that jedi does not propose .cache as a completion as Jedi does not complete path.

screen shot 2017-01-18 at 21 38 20

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 20, 2017

Member

@takluyver I have a feeling you'll have opinions on this one. As a first pass to ward jedi integration and high level views. Thoughs ?

Member

Carreau commented Jan 20, 2017

@takluyver I have a feeling you'll have opinions on this one. As a first pass to ward jedi integration and high level views. Thoughs ?

@pylang

This comment has been minimized.

Show comment
Hide comment
@pylang

pylang Jan 21, 2017

hmm. Jedi. You mentioned to ask how can we help. Anything I can do? : )

pylang commented Jan 21, 2017

hmm. Jedi. You mentioned to ask how can we help. Anything I can do? : )

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 21, 2017

Member

hmm. Jedi. You mentioned to ask how can we help. Anything I can do? : )

Sure ! How comfortable are you with Python ?

Right now I need to know if:

  • What I wrote make sens.
  • Help writing some documentation
  • work in parallel on a patch to ipykernel repo (ipykernel.py, do_complete method) for this to be availlablein the notebook, and figureout a way to work both with the old and new API.

Other things that should be worked on: assuming you do im<tab>
Jedi will tell to replace chars 0 to 2 with import. While current completion machinery will say replace2 to 2 py port. Obviously the 2 completions are the same, but the deduplication logic does not handle this.

The current API return text:str, matches:List[str], the jedi based completion are discarded by the old API (because they are of different form). It would be nice to figure out how to convert from the Jedi way to the old API.

The old API is also relatively not well documented so understanding (and documenting) would be great.

Lastly there are a number of issues tagged tab-completion on this repo; I'm pretty sure a number of them should be fixed by this pull-request. We should go through them and look at which ones would be fixed by this.

In a first iteration if you already managed to get that to work on your machine, and test to find bugs, that would be super-helpful !

Also I'm testing with Jedi-dev, I need to make sure it also work with Jedi stable !

Here is a high level view, if you have any more detailed questions feel free to ask !

Member

Carreau commented Jan 21, 2017

hmm. Jedi. You mentioned to ask how can we help. Anything I can do? : )

Sure ! How comfortable are you with Python ?

Right now I need to know if:

  • What I wrote make sens.
  • Help writing some documentation
  • work in parallel on a patch to ipykernel repo (ipykernel.py, do_complete method) for this to be availlablein the notebook, and figureout a way to work both with the old and new API.

Other things that should be worked on: assuming you do im<tab>
Jedi will tell to replace chars 0 to 2 with import. While current completion machinery will say replace2 to 2 py port. Obviously the 2 completions are the same, but the deduplication logic does not handle this.

The current API return text:str, matches:List[str], the jedi based completion are discarded by the old API (because they are of different form). It would be nice to figure out how to convert from the Jedi way to the old API.

The old API is also relatively not well documented so understanding (and documenting) would be great.

Lastly there are a number of issues tagged tab-completion on this repo; I'm pretty sure a number of them should be fixed by this pull-request. We should go through them and look at which ones would be fixed by this.

In a first iteration if you already managed to get that to work on your machine, and test to find bugs, that would be super-helpful !

Also I'm testing with Jedi-dev, I need to make sure it also work with Jedi stable !

Here is a high level view, if you have any more detailed questions feel free to ask !

@pylang

This comment has been minimized.

Show comment
Hide comment
@pylang

pylang Jan 21, 2017

I can try bug testing and reading the code. Thanks.

pylang commented Jan 21, 2017

I can try bug testing and reading the code. Thanks.

@Carreau Carreau changed the title from [WIP] Reintegrate jedi to Reintegrate jedi Jan 24, 2017

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 24, 2017

Member

Ok, I think this is ready-ish for review, with tests, docs. I rebased/squashed everything and here is the commit message for the current commit:


First step in reintegrating Jedi

If Jedi is installed expose a private API use it with prompt toolkit.
Jedi does not yet provide all the completion IPython has, so this is
still a bit awkward. In order to debug this (and see what is Jedi
provided we for now inject a fake Jedi/IPython delimiter in the menu.

Jedi completion and this behavior are enabled by default, but could
likely be opt-in.

Add also a number of debug flags to be able to track why jedi is not
working, and/or what completions are found by IPython and not Jedi.
That should give us a bit of heads up and feedback to know whether we
can remove part of the IPython completer, and more especially if we can
drop python_matches.

Once python_matches is dropped and some other of the current matchers
are either dropped or converted to the new API, that should simplify the
internal quite a bit.

That would just be too much for an already BIG pull-request.


@willingc I did a pass on documenting thing better, I would appreciate hints on where to do things better.

Member

Carreau commented Jan 24, 2017

Ok, I think this is ready-ish for review, with tests, docs. I rebased/squashed everything and here is the commit message for the current commit:


First step in reintegrating Jedi

If Jedi is installed expose a private API use it with prompt toolkit.
Jedi does not yet provide all the completion IPython has, so this is
still a bit awkward. In order to debug this (and see what is Jedi
provided we for now inject a fake Jedi/IPython delimiter in the menu.

Jedi completion and this behavior are enabled by default, but could
likely be opt-in.

Add also a number of debug flags to be able to track why jedi is not
working, and/or what completions are found by IPython and not Jedi.
That should give us a bit of heads up and feedback to know whether we
can remove part of the IPython completer, and more especially if we can
drop python_matches.

Once python_matches is dropped and some other of the current matchers
are either dropped or converted to the new API, that should simplify the
internal quite a bit.

That would just be too much for an already BIG pull-request.


@willingc I did a pass on documenting thing better, I would appreciate hints on where to do things better.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 25, 2017

Member

There is a duplication issue of import in the from foo im<tab> case.

Member

Carreau commented Jan 25, 2017

There is a duplication issue of import in the from foo im<tab> case.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 26, 2017

Member

There is a duplication issue of import in the from foo im case.

Ok, that's an edge case and not a bug. One completer returns import with space, the other without space so they are different. One we drop python_matches that will be self-solved.

Member

Carreau commented Jan 26, 2017

There is a duplication issue of import in the from foo im case.

Ok, that's an edge case and not a bug. One completer returns import with space, the other without space so they are different. One we drop python_matches that will be self-solved.

Carreau added some commits Jan 18, 2017

First step in reintegrating Jedi
If Jedi is installed expose a private API use it with prompt toolkit.
Jedi does not _yet_ provide all the completion IPython has, so this is
still a bit awkward. In order to debug this (and see what is Jedi
provided we for now inject a fake Jedi/IPython delimiter in the menu.

Jedi completion and this behavior are enabled by default, but could
likely be opt-in.

Add also a number of debug flags to be able to track why jedi is not
working, and/or what completions are found by IPython and not Jedi.
That should give us a bit of heads up and feedback to know whether we
can remove part of the IPython completer, and more especially if we can
drop `python_matches`.

Once `python_matches` is dropped and some other of the current matchers
are either dropped or converted to the new API, that should simplify the
internal quite a bit.

That would just be too much for an already BIG pull-request.
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jan 31, 2017

Member

Rebased.

Member

Carreau commented Jan 31, 2017

Rebased.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Feb 1, 2017

Member

Signing up for these completion threads -- is there a good way for me to run the changes you have for ipykernel and ipython together? Check out the branches for both of these into a virtual environment and set them up as a kernel?

Member

rgbkrk commented Feb 1, 2017

Signing up for these completion threads -- is there a good way for me to run the changes you have for ipykernel and ipython together? Check out the branches for both of these into a virtual environment and set them up as a kernel?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 1, 2017

Member

Signing up for these completion threads -- is there a good way for me to run the changes you have for ipykernel and ipython together? Check out the branches for both of these into a virtual environment and set them up as a kernel?

I hope someone to merge that soonish, I'll update my ipykernel branch then.

Yes checking the branches in an env would work. The ipykernel one does not sed his extra info in metadata, but that should "just" take me 30 min to do

Member

Carreau commented Feb 1, 2017

Signing up for these completion threads -- is there a good way for me to run the changes you have for ipykernel and ipython together? Check out the branches for both of these into a virtual environment and set them up as a kernel?

I hope someone to merge that soonish, I'll update my ipykernel branch then.

Yes checking the branches in an env would work. The ipykernel one does not sed his extra info in metadata, but that should "just" take me 30 min to do

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Feb 1, 2017

Member

Because I saw in another thread that you're frustrated by the wait for review: this is on my queue to look at, but it falls into the same trap I told Mike about. Things that I know I want longer to look at tend to get pushed down the queue, because bashing through the easy issues is more effective in keeping down the number waiting for attention.

Member

takluyver commented Feb 1, 2017

Because I saw in another thread that you're frustrated by the wait for review: this is on my queue to look at, but it falls into the same trap I told Mike about. Things that I know I want longer to look at tend to get pushed down the queue, because bashing through the easy issues is more effective in keeping down the number waiting for attention.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 1, 2017

Member

Because I saw in another thread that you're frustrated by the wait for review: this is on my queue to look at, but it falls into the same trap I told Mike about. Things that I know I want longer to look at tend to get pushed down the queue, because bashing through the easy issues is more effective in keeping down the number waiting for attention.

Thanks, I appreciate, and I understand. I've tried to push your PR and notebook 5.0 issues as well to give you some review time.

Member

Carreau commented Feb 1, 2017

Because I saw in another thread that you're frustrated by the wait for review: this is on my queue to look at, but it falls into the same trap I told Mike about. Things that I know I want longer to look at tend to get pushed down the queue, because bashing through the easy issues is more effective in keeping down the number waiting for attention.

Thanks, I appreciate, and I understand. I've tried to push your PR and notebook 5.0 issues as well to give you some review time.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Feb 2, 2017

Member

I've had a brief scan through this; it looks OK. As before, I think the main thing is to land it in master and start trying it out.

The provisional context manager is clever, but I'm not convinced that it's that useful for this. Most consumers are using it as part of IPython, not the API.

Member

takluyver commented Feb 2, 2017

I've had a brief scan through this; it looks OK. As before, I think the main thing is to land it in master and start trying it out.

The provisional context manager is clever, but I'm not convinced that it's that useful for this. Most consumers are using it as part of IPython, not the API.

@rgbkrk

rgbkrk approved these changes Feb 2, 2017

@takluyver takluyver merged commit e1e2e96 into ipython:master Feb 3, 2017

4 checks passed

codecov/patch 75.3% of diff hit (target 0%)
Details
codecov/project 66.38% (+0.23%) compared to 1347d9a
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.

Show comment
Hide comment
@takluyver

takluyver Feb 3, 2017

Member

Let's see what happens :-)

Member

takluyver commented Feb 3, 2017

Let's see what happens :-)

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 3, 2017

Member

Thank you !

Member

Carreau commented Feb 3, 2017

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment