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] Fix #471 Parallel #602

Merged
merged 41 commits into from
Feb 4, 2018
Merged

[RFC] Fix #471 Parallel #602

merged 41 commits into from
Feb 4, 2018

Conversation

Shougo
Copy link
Owner

@Shougo Shougo commented Dec 24, 2017

It is on WIP.

Note: profile and debug feature is not supported.

You can test the feature!

@Shougo Shougo force-pushed the parallel branch 4 times, most recently from 536ef98 to 8995d3a Compare December 24, 2017 09:02
@Shougo
Copy link
Owner Author

Shougo commented Dec 25, 2017

The current status is:

The refactoring before threading is done.
The next is threading support.

@Shougo
Copy link
Owner Author

Shougo commented Dec 26, 2017

Note: profile and debug feature is not supported.

I will fix the problem later.

@Shougo
Copy link
Owner Author

Shougo commented Jan 1, 2018

@roxma I have added the threading feature.
But it does not work...
Can you test the plugin?

@Shougo
Copy link
Owner Author

Shougo commented Jan 1, 2018

attach vim in another thread seems does not work.
It works on nvim-completion-manager though.

@Shougo
Copy link
Owner Author

Shougo commented Jan 8, 2018

I have updated it.
It still WIP though.

@Shougo
Copy link
Owner Author

Shougo commented Jan 8, 2018

I have implemented the feature.
But it will be crashed by Out of memory.
Why?

@Shougo
Copy link
Owner Author

Shougo commented Jan 8, 2018

The more testers and the code reviewers are needed.

@Mte90
Copy link

Mte90 commented Jan 10, 2018

I can help to test on vim in case :-) What is the way to get that branch with vundle?

@deathmaz
Copy link

any configuration needed to make it to work? i'm on linux and it didn't work when i switched the branch

@Shougo
Copy link
Owner Author

Shougo commented Jan 11, 2018

I can help to test on vim in case :-) What is the way to get that branch with vundle?

Vundle does not support branch swich.
You can switch the branch manually.

any configuration needed to make it to work? i'm on linux and it didn't work when i switched the branch

No additional configuration is needed.
Note: It is very experimental. It is slow. You must wait few second.

@deathmaz
Copy link

@Shougo yes, it works after i wait for a few seconds, do you need to test some specific things?

@Shougo
Copy link
Owner Author

Shougo commented Jan 11, 2018

@Shougo yes, it works after i wait for a few seconds, do you need to test some specific things?

Nothing. It works. But it is very experimental.

I'm waiting for neovim/pynvim#294.

@Shougo
Copy link
Owner Author

Shougo commented Jan 24, 2018

I will change the implementation using msgpack instead of Vim variables.

@Shougo Shougo changed the title WIP: Fix #471 Parallel : Fix #471 Parallel Jan 28, 2018
@Shougo Shougo changed the title : Fix #471 Parallel RFC: Fix #471 Parallel Jan 28, 2018
@Shougo
Copy link
Owner Author

Shougo commented Jan 28, 2018

I have completed the development.
More testers are needed.
Especially Windows environment.

@blueyed Profile and debug feature tests are needed.

@Shougo
Copy link
Owner Author

Shougo commented Jan 31, 2018

I will add the error check later.
I think python exe(get(g:, 'python3_host_prog', 'python3')) is not in $PATH.

@jsfaint
Copy link

jsfaint commented Jan 31, 2018

@Shougo Sorry, I checked the vimrc, the g:python3_host_prog is only set for vim

if !has('nvim')
  let g:python3_host_prog = 'C:/Users/jia.sui/AppData/Local/Programs/Python/Python36/python.exe'
endif

After set the g:python3_host_prog and then try to use the completion.
There are 4 python processes eat up my CPU, and there is no completion window showed up
image

@Shougo
Copy link
Owner Author

Shougo commented Jan 31, 2018

I don't know why it does not work.
You can research the problem.

Note: You have seen the help wanted label.
deoplete is Python3 based. It is easy to contribute.

@balta2ar
Copy link
Contributor

You're using msgpack here. What's the reasoning for this choice? Could it be JSON or Pickle instead? I'm not questioning the choice, it's just good for my own knowledge to know which one to pick in which situation.

@Shougo
Copy link
Owner Author

Shougo commented Jan 31, 2018

You're using msgpack here. What's the reasoning for this choice? Could it be JSON or Pickle instead? I'm not questioning the choice, it's just good for my own knowledge to know which one to pick in which situation.

It is good question.
I have choosed the library because:

  • neovim uses it(No additional dependency).
  • It supports stream decode. It is useful for standard input/output communication.
  • The decode/encode performance is good than json.

@Shougo Shougo merged commit 0a6debf into master Feb 4, 2018
@Shougo Shougo deleted the parallel branch February 4, 2018 09:05
@Shougo
Copy link
Owner Author

Shougo commented Feb 4, 2018

Merged.

@balta2ar
Copy link
Contributor

balta2ar commented Feb 5, 2018

Unfortunately, it broke my deoplete plugin...

[deoplete] Traceback (most recent call last):
[deoplete]   File "/home/user/.vim/plugged/deoplete.nvim/rplugin/python3/deoplete/deoplete.py", line 63, in completion_begin
[deoplete]     is_async, position, candidates = self.merge_results(context)
[deoplete]   File "/home/user/.vim/plugged/deoplete.nvim/rplugin/python3/deoplete/deoplete.py", line 110, in merge_results
[deoplete]     result = parent.merge_results(context)
[deoplete]   File "/home/user/.vim/plugged/deoplete.nvim/rplugin/python3/deoplete/parent.py", line 51, in merge_results
[deoplete]     get = self._get(queue_id)
[deoplete]   File "/home/user/.vim/plugged/deoplete.nvim/rplugin/python3/deoplete/parent.py", line 91, in _get
[deoplete]     return [x for x in self._proc.communicate(15)
[deoplete]   File "/home/user/.vim/plugged/deoplete.nvim/rplugin/python3/deoplete/parent.py", line 92, in <listcomp>
[deoplete]     if x['queue_id'] == queue_id]
[deoplete] TypeError: 'int' object is not subscriptable
[deoplete] Error while gathering completions.  Use :messages for error details.

@Shougo
Copy link
Owner Author

Shougo commented Feb 5, 2018

@balta2ar I cannot reproduce the problem.
Please create the new issue. We have not ESP skills.
Otherwise, I will ignore your issue.

Unfortunately, it broke my deoplete plugin...

What is the plugin?

@balta2ar
Copy link
Contributor

balta2ar commented Feb 6, 2018

It looks like I found the problem in my code: I had several prints here and there in my plugin, so their output of course was unexpected to the Parent class. I removed prints, and the errors were gone. I know it's not the issue of the plugin, but is there anything that can be done to help find such problems easier? How about printing a friendly message saying "Unexpected output from child process. Are you printing something to stdout there?". That's just an idea though.

But there is still a problem. My plugin relies on the is_async flag logic: I expect that deoplete will keep calling my gather_candidates method until I set context["is_async"] to False. When I process data in the background, I keep setting is_async to True. But it appears that deoplete does not call me again when my results are ready. Could you please check this behavior?

@Shougo
Copy link
Owner Author

Shougo commented Feb 6, 2018

How about printing a friendly message saying "Unexpected output from child process. Are you printing something to stdout there?". That's just an idea though.

If you can implement the feature, should be.
The PR is wellcome.

But it appears that deoplete does not call me again when my results are ready. Could you please check this behavior?

Hm. I will check the behavior later.
Please create the new issue for it.

@bfredl
Copy link
Contributor

bfredl commented Feb 6, 2018

To support using print() in children could use stdout redirection (in the child) like the script host: https://github.com/neovim/python-client/blob/320cb7d92c2bfaabe10bd278efd99361ebabc2bf/neovim/plugin/script_host.py#L64

@bfredl
Copy link
Contributor

bfredl commented Feb 6, 2018

Or maybe just sys.stdout = sys.stderr, and then handle stderr in the parent...

@Shougo
Copy link
Owner Author

Shougo commented Feb 6, 2018

OK. I will test that.

@Shougo
Copy link
Owner Author

Shougo commented Feb 7, 2018

diff --git a/rplugin/python3/deoplete/child.py b/rplugin/python3/deoplete/child.py
index 1e14e09..464e1fd 100644
--- a/rplugin/python3/deoplete/child.py
+++ b/rplugin/python3/deoplete/child.py
@@ -79,10 +79,13 @@ class Child(logger.LoggingMixin):
                     self._on_event(args[0])
                 elif name == 'merge_results':
                     self._write(self._merge_results(args[0], queue_id))
+                self.debug('main_loop: end')
 
     def _write(self, expr):
-        sys.stdout.buffer.write(self._packer.pack(expr))
-        sys.stdout.flush()
+        # sys.stdout.buffer.write(self._packer.pack(expr))
+        # sys.stdout.flush()
+        sys.stderr.buffer.write(self._packer.pack(expr))
+        sys.stderr.flush()
 
     def _enable_logging(self):
         logging = self._vim.vars['deoplete#_logging']
@@ -141,6 +144,7 @@ class Child(logger.LoggingMixin):
                 self.debug('Loaded Filter: %s (%s)', f.name, path)
 
     def _merge_results(self, context, queue_id):
+        self.debug('merged_results: begin')
         results = self._gather_results(context)
 
         merged_results = []
@@ -162,6 +166,7 @@ class Child(logger.LoggingMixin):
 
         is_async = len([x for x in results if x['context']['is_async']]) > 0
 
+        self.debug('merged_results: end')
         return {
             'queue_id': queue_id,
             'is_async': is_async,

diff --git a/rplugin/python3/deoplete/dp_main.py b/rplugin/python3/deoplete/dp_main.py
index 04e0448..dff1e84 100644
--- a/rplugin/python3/deoplete/dp_main.py
+++ b/rplugin/python3/deoplete/dp_main.py
@@ -5,6 +5,7 @@
 # ============================================================================
 
 import sys
+import io
 
 from neovim import attach
 
@@ -28,10 +29,21 @@ def attach_vim(serveraddr):
     return vim
 
 
+class RedirectStream(io.IOBase):
+    def __init__(self, redirect_handler):
+        self.redirect_handler = redirect_handler
+
+    def write(self, data):
+        self.redirect_handler(data)
+
+    def writelines(self, seq):
+        self.redirect_handler('\n'.join(seq))
+
 def main(serveraddr):
     vim = attach_vim(serveraddr)
     from deoplete.child import Child
     from deoplete.util import error_tb
+    sys.stdout = RedirectStream(lambda data: vim.out_write(data))
     try:
         child = Child(vim)
         child.main()
diff --git a/rplugin/python3/deoplete/process.py b/rplugin/python3/deoplete/process.py
index d1ac7b3..e81ca35 100644
--- a/rplugin/python3/deoplete/process.py
+++ b/rplugin/python3/deoplete/process.py
@@ -52,7 +52,11 @@ class Process(object):
 
     def enqueue_output(self):
         while self._proc:
-            b = self._proc.stdout.read(1)
+            # b = self._proc.stdout.raw.read(102400)
+            b = self._proc.stderr.raw.read(102400)
+            if b == b'':
+                return
+
             self._unpacker.feed(b)
             for child_out in self._unpacker:
                 self._queue_out.put(child_out)

Hm. It may be better.

@Shougo
Copy link
Owner Author

Shougo commented Feb 11, 2018

@bfredl I cannot use stderr because of SpaceVim/SpaceVim#1397

@Shougo
Copy link
Owner Author

Shougo commented Feb 11, 2018

@balta2ar I have fixed stdout problem completely.

@bfredl
Copy link
Contributor

bfredl commented Feb 11, 2018

Why are you sending the msgpack over stderr? That will indeed cause all sorts of trouble... Instead, use the original stdout stream:

self.stdout = sys.stdout # use it for msgpack
sys.stdout = RedirectStream(...) # gets print() out of the way

@Shougo
Copy link
Owner Author

Shougo commented Feb 12, 2018

Yes. I have fixed it in the latest version.

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

Successfully merging this pull request may close these issues.

8 participants