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

[RFC] Use a http wrapper for jedi #160

Closed
wants to merge 22 commits into from
Closed

[RFC] Use a http wrapper for jedi #160

wants to merge 22 commits into from

Conversation

vheon
Copy link
Contributor

@vheon vheon commented Jun 17, 2015

As a first step for supporting python2 and python3 I wrapped jedi with JSON+HTTP using Bottle. This approach has been discussed previously in ycm-core/YouCompleteMe#1278, #117, #119. After this the next step will be to add a multiplexer for two JediHTTP (one for python2 and one for python3); so will be easier to review and reason about it. I don't know if it is production ready so I'd really love a first review before continuing to work in the (possibly) wrong direction.

PS: I'm not really a python dev so if someone will give a review of the JediHTTP wrapper itself it would be great.

I've signed the CLA.

Review on Reviewable

@vheon vheon changed the title Use a http wrapper for jedi [RFC] Use a http wrapper for jedi Jun 17, 2015
@Valloric
Copy link
Member

I gave this a light review; the general direction seems fine. Nice work!

@homu
Copy link
Contributor

homu commented Jul 14, 2015

☔ The latest upstream changes (presumably #167) made this pull request unmergeable. Please resolve the merge conflicts.

PATH_TO_PYTHON = sys.executable
PATH_TO_JEDIHTTP = os.path.join(
os.path.abspath( os.path.dirname( __file__ ) ),
'..', '..', '..', 'third_party', 'JediHTTP', 'jedihttp' )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to use os.path.pardir instead of hardcoding '..'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the doc I read that pardir is '..' for Windows and POSIX which is all the necessary for ycmd. Plus we use this pattern over all ycmd codebase. What would be the advantage of using pardir?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I was mostly thinking of compatibility, but since they use the same string anyway it's probably not very important. I guess the advantage would be if some platform used something other than .. it will just work (given that Python supports that platform of course).

@reinhrst
Copy link

Very well written I think. Not entirely sure if/how I can update your pull request, I resolved the conflict to current master, and fixed a test in https://github.com/reinhrst/ycmd/tree/feature-jedihttp . Haven't actually tried running this code; will do that next time I have a spare hour.

Two small things come to mind that may still need a bit work:

  • Is it secure enough? Although it's running on a random port, should perhaps all requests be protected with a secret, as in the case of ycmd itself?
  • Right now the python executable for jediHTTPis the same as the one used for ycmd, which is not what we want. Does it make sense to just set this to python, meaning it will pick up the python version of whatever virtualenv it's in, or do we need something smarter?

Finally, not sure I follow you on the "multiplexer for two JediHTTP" remark.

@reinhrst
Copy link

Installed https://github.com/reinhrst/ycmd/tree/feature-jedihttp into my vim this morning, with the one change that I replaced sys.executable in jedi_completer.py with "python", and started my vim after activating my virtualenv (python 3.4.3). I can confirm that I get suggestions both for built-in python3 modules (asyncio), and for tornado which is not installed anywhere but in that virtualenv. This is far from an exhaustive test, but so far I'm very happy with the result!

@vheon
Copy link
Contributor Author

vheon commented Sep 15, 2015

Is it secure enough? Although it's running on a random port, should perhaps all requests be protected with a secret, as in the case of ycmd itself?

A actually don't know: we use other completion engine like Omnisharp, TSServer and to some extent gocode and we don't use any secure measure with them so at the moment I didn't considered it.

Right now the python executable for jediHTTPis the same as the one used for ycmd, which is not what we want. Does it make sense to just set this to python, meaning it will pick up the python version of whatever virtualenv it's in, or do we need something smarter?

You are absolutely right, with the current state of the PR we pick the same Python as the one used by ycmd and the reason is that I didn't want to bring new feature (use different Python executables) with architectural changes (use a http wrapper instead of calling Jedi directly) at the same time.

The roadmap I see is the following:

  • change the architecture of the completed from direct call to Jedi to http wrapper.
  • detect the Python executable and spawn a JediHTTP using that executable.
  • make the ycmd Python completer able to communicate with multiple instances of JediHTTP, in that way we can complete Python2 in a buffer and Python3 in another of the same instance of vim.

PS: I'm on vacation right now so I will not be able to respond to other concerns until Sunday.

@reinhrst
Copy link

change the architecture of the completed from direct call to Jedi to http wrapper.

I think this is the current stage of the PR (with my 2 small changes)

detect the Python executable and spawn a JediHTTP using that executable.

I think it's a combination of python executable (which defines the python version) and environment variables (which define the virtualenv). A virtualenv changes the $PATH variable to make sure python points to the right executable. Do you think we need anything smarter, than just picking up these from whatever is set for vim at the time of server restart?

make the ycmd Python completer able to communicate with multiple instances of JediHTTP, in that way we can complete Python2 in a buffer and Python3 in another of the same instance of vim.

I feel that the first step here is to think of a good way to specify what exe/virtualenv is needed for each file. I think for now we have a very releasable version with just the first 2 points solved.

PS: I'm on vacation right now so I will not be able to respond to other concerns until Sunday.

Have fun :)

@reinhrst
Copy link

How will we move forward on this? I've been running this code for the past 2 weeks without noticing any problems, I would argue that it is a great improvement over the current state of things, especially since virtualenvs and python3 are getting more and more popular.

@vheon
Copy link
Contributor Author

vheon commented Sep 27, 2015

I've got actually news about this. Finally I got the time to ask permission for continuing working actively on this and I got it and if fact I'm working on it right now. I'm working on updating the code: since I made this PR things got added, like the Fallback from Semantic to Identifier based completion and the GetDoc subcommand. I've got some problem with the github repo so I think I'm going to close this PR and open another one. Anyway at the moment I'm going to address only the first point of my roadmap:

change the architecture of the completed from direct call to Jedi to http wrapper.

@reinhrst I wanted to thank you for trying this out, and wanted to ask you if you were keen to continue testing this transition to the new architecture (I don't really work on python projects, except the python section of this and the YCM client ;) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants