Skip to content

[RFC] Fix #471 Parallel - #602

Merged
Shougo merged 41 commits into
masterfrom
parallel
Feb 4, 2018
Merged

[RFC] Fix #471 Parallel#602
Shougo merged 41 commits into
masterfrom
parallel

Conversation

@Shougo

@Shougo Shougo commented Dec 24, 2017

Copy link
Copy Markdown
Owner

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

Shougo commented Dec 25, 2017

Copy link
Copy Markdown
Owner Author

The current status is:

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

@Shougo

Shougo commented Dec 26, 2017

Copy link
Copy Markdown
Owner Author

Note: profile and debug feature is not supported.

I will fix the problem later.

@Shougo

Shougo commented Jan 1, 2018

Copy link
Copy Markdown
Owner Author

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

@Shougo

Shougo commented Jan 1, 2018

Copy link
Copy Markdown
Owner Author

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

@Shougo

Shougo commented Jan 8, 2018

Copy link
Copy Markdown
Owner Author

I have updated it.
It still WIP though.

@Shougo

Shougo commented Jan 8, 2018

Copy link
Copy Markdown
Owner Author

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

@Shougo

Shougo commented Jan 8, 2018

Copy link
Copy Markdown
Owner Author

The more testers and the code reviewers are needed.

@Mte90

Mte90 commented Jan 10, 2018

Copy link
Copy Markdown

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

@deathmaz

Copy link
Copy Markdown

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

@Shougo

Shougo commented Jan 11, 2018

Copy link
Copy Markdown
Owner Author

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
Copy Markdown

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

@Shougo

Shougo commented Jan 11, 2018

Copy link
Copy Markdown
Owner Author

@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

Shougo commented Jan 24, 2018

Copy link
Copy Markdown
Owner Author

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

Shougo commented Jan 28, 2018

Copy link
Copy Markdown
Owner Author

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

@blueyed Profile and debug feature tests are needed.

@Shougo

Shougo commented Jan 31, 2018

Copy link
Copy Markdown
Owner Author

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

@jsfaint

jsfaint commented Jan 31, 2018

Copy link
Copy Markdown

@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

Shougo commented Jan 31, 2018

Copy link
Copy Markdown
Owner Author

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
Copy Markdown
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

Shougo commented Jan 31, 2018

Copy link
Copy Markdown
Owner Author

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

Shougo commented Feb 4, 2018

Copy link
Copy Markdown
Owner Author

Merged.

@balta2ar

balta2ar commented Feb 5, 2018

Copy link
Copy Markdown
Contributor

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

Shougo commented Feb 5, 2018

Copy link
Copy Markdown
Owner Author

@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

balta2ar commented Feb 6, 2018

Copy link
Copy Markdown
Contributor

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

Shougo commented Feb 6, 2018

Copy link
Copy Markdown
Owner Author

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

bfredl commented Feb 6, 2018

Copy link
Copy Markdown
Contributor

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

bfredl commented Feb 6, 2018

Copy link
Copy Markdown
Contributor

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

@Shougo

Shougo commented Feb 6, 2018

Copy link
Copy Markdown
Owner Author

OK. I will test that.

@Shougo

Shougo commented Feb 7, 2018

Copy link
Copy Markdown
Owner Author
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

Shougo commented Feb 11, 2018

Copy link
Copy Markdown
Owner Author

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

@Shougo

Shougo commented Feb 11, 2018

Copy link
Copy Markdown
Owner Author

@balta2ar I have fixed stdout problem completely.

@bfredl

bfredl commented Feb 11, 2018

Copy link
Copy Markdown
Contributor

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

Shougo commented Feb 12, 2018

Copy link
Copy Markdown
Owner Author

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