Environment: improve/fix getting sys.prefix #1108

blueyed · 2018-04-27T08:08:05Z

Used with jedi-vim: davidhalter/jedi-vim#836
Used with deoplete-jedi: deoplete-plugins/deoplete-jedi#184

davidhalter · 2018-05-01T22:18:47Z

I like the idea of fixing all of this.

I just think we should probably do it right. We should probably just start the subprocess and work with it instead of trying stuff with -c.

blueyed · 2018-05-02T13:47:08Z

Let's move it into EvaluatorSubprocess then?

davidhalter · 2018-05-02T21:34:01Z

I guess, yes. Probably to _CompiledSubprocess. There's already a get_sys_path function there that is being used, so we could use it as well for prefix and version.

blueyed

Another unsubmitted pending review.

blueyed · 2018-04-27T08:30:35Z

jedi/api/environment.py

@@ -55,40 +55,53 @@ class Environment(_BaseEnvironment):
    should not create it directly. Please use create_environment or the other
    functions instead. It is then returned by that function.
    """
-    def __init__(self, path, executable):
-        self.path = os.path.abspath(path)
+    def __init__(self, executable):


Maybe change the signature to def __init__(self, exe, executable=None, prefix=None, version_info=None): and then make executable, prefix and version_info lazy-evaluated and memoized properties?
Would allow to specify some of the properties directly, and get others once on demand.

Hmm, what's the advantage of specifying all those things? I would probably prefer simplicity. If we have to check it anyway for some cases, checking it for all cases probably makes sense. (It doesn't take a lot of time, what takes a lot of time is to start a Python interpreter).

Everythings gets retrieved in one go now.

blueyed · 2018-04-27T08:30:54Z

jedi/api/environment.py

-    def __init__(self, path, executable):
-        self.path = os.path.abspath(path)
+    def __init__(self, executable):
+        self.path = None


path might get renamed to prefix for consistency.

I totally agree. The only problem is that the API is alrady out there. But we can probably just deprecate self.path.

blueyed · 2018-05-10T18:04:50Z

Just a side-note: for jedi-vim it would be beneficial to get an Environment based on the exe only.

I will now look into addressing your comments / improve this.

davidhalter · 2018-05-10T20:52:06Z

I agree. It's probably generally better for a lot of use cases to just specify an executable.

For virtualenvs it's sometimes more practical to specify a prefix, but that could just try to locate the exe and then progress like the exe mechanism.

blueyed · 2018-06-23T09:34:56Z

Refactored, please review.
The jedi-vim part: davidhalter/jedi-vim#836.

davidhalter · 2018-06-23T12:47:09Z

That already looks way better. I'll have a closer look in the next few days.

davidhalter · 2018-06-30T12:47:12Z

jedi/api/environment.py

@@ -246,23 +242,6 @@ def find_system_environments():
            pass


_get_version was deleted in the super class, so we can remove it here safely as well. However SameEnvironment shouldn't execute anything (It's mostly there for REPL completions).

I think we should probably be doing something like this:

class Environment(object): def __init__(self, executable): self.executable = executable self._load_attributes() def _load_attributes(self): ... # subprocess stuff class SameEnvironment(): def __init__(self): super(SameEnvironment, self).__init__(sys.executable) def _load_attributes(self): self.version_info = sys.version_info self.path = sys.prefix

I think it can be simpler - pushed 2bb32e5.

davidhalter · 2018-06-30T16:51:07Z

E           jedi.api.environment.InvalidPythonEnvironment: Could not get version information (TypeError("get_subprocess() missing 1 required positional argument: 'version'",))

blueyed · 2018-06-30T17:09:45Z

@davidhalter
More context please.
(there is no version argument, is there?!

jedi/jedi/evaluate/compiled/subprocess/__init__.py

Line 32 in 2bb32e5

def get_subprocess(executable):

)

blueyed · 2018-06-30T17:11:03Z

Oh, tests - I have not looked into fixing them yet.
Wanted to get your feedback on this in general first.

blueyed · 2018-06-30T17:49:57Z

This is because of 282c6a2.
There is a chicken-egg problem now: we use the subprocess to get the version, but that requires the version already (for the pickle protocol).
I think we need to start with the lowest protocol version initially (#1158).
/cc @micbou

blueyed · 2018-06-30T18:11:12Z

jedi/api/environment.py

        """
-        self.executable = os.path.abspath(executable)
+        self.path = info[1]


@davidhalter
Should we keep path here, or remove, or deprecate it instead of self.prefix?

Keep path for now. I think we should have a discussion about prefix in a separate PR or issue. I probably think we should deprecate it.

micbou · 2018-06-30T20:21:47Z

I see the problem. I would suggest to either revert PR #1152 or print sys.version and sys.prefix through the Python interpreter and parse the output.

blueyed · 2018-06-30T21:16:40Z

@micbou
Reverting is not necessary.
I think the server should pass its version to the client/subprocess, and then both would use highest_pickle_protocol with their own version and the other.

The internal protocol is not really clear to me, i.e could the instance be made available in the callback?
Then _get_info could set the pickle protocol on the server already and return what it sets.
(For now I've came across #1159 - which might simplify it a bit.)

davidhalter · 2018-06-30T23:45:37Z

If it's too much of a hassle we can also just revert #1152. It's still possible to reintroduce it at a later stage. I think this PR is way more important.

Generally if you want to make the pickle version 4 work in certain cases don't create too much complexity for it. It's really not worth it. IMO it's almost too much code for the effect it has at the moment. Please don't add way more code to pick a pickle version than it uses now.

micbou · 2018-07-05T14:42:19Z

I think the server should pass its version to the client/subprocess, and then both would use highest_pickle_protocol with their own version and the other.

Except that the server also needs the version of the client. How are you going to pass that version from the client to the server?

blueyed · 2018-07-05T14:50:20Z

@micbou
It gets retrieved via the _get_info function, used when setting up the connection/Environment:
https://github.com/davidhalter/jedi/pull/1108/files#diff-c204f420c3d90e6c005f6354c65dc638R47

micbou · 2018-07-05T15:37:18Z

@blueyed So the plan is to send _get_info with the lowest pickle protocol version (i.e. version 2), get the version of the client, compute the highest pickle protocol version, and use that version for all subsequent communications?

I have tried to implement restarting in case `g:deoplete#sources#jedi#python_path` gets changed, and figured that it is way easier when using only Jedi. There are likely previous features missing, and it needs cleaning up / deleting still. It uses the Jedi branch to improve handling of Environments: davidhalter/jedi#1108

blueyed · 2018-07-08T19:51:51Z

Any idea about the test_venv_and_pths failures for all builds != JEDI_TEST_ENVIRONMENT=27 ?

I have tried to implement restarting in case `g:deoplete#sources#jedi#python_path` gets changed, and figured that it is way easier when using only Jedi. There are likely previous features missing, and it needs cleaning up / deleting still. It uses the Jedi branch to improve handling of Environments: davidhalter/jedi#1108

blueyed · 2018-07-08T23:51:54Z

Are there plans to clean old environments already?
We should probably remember their starttime already, and when they are used?

Then maybe by default only the last X recently used should be kept running? (to be checked when a new environment is created)

I have tried to implement restarting in case `g:deoplete#sources#jedi#python_path` gets changed, and figured that it is way easier when using only Jedi. There are likely previous features missing, and it needs cleaning up / deleting still. It uses the Jedi branch to improve handling of Environments: davidhalter/jedi#1108

davidhalter · 2018-07-09T19:00:13Z

Are there plans to clean old environments already?
We should probably remember their starttime already, and when they are used?

No we currently don't do that. I would just wait until this one is merged (or just do it in another branch), so this doesn't get too big.

davidhalter · 2018-07-09T22:29:05Z

I'm trying to pin down the remaining error. Have been searching for 2 hours, no luck so far. Will continue tomorrow while commuting.

davidhalter · 2018-07-10T08:11:42Z

I fixed it in d06e55a it's on master, please rebase.

davidhalter · 2018-07-10T08:12:38Z

You are probably finished, right? Everything else could go into a different PR?

It only takes `executable` and gets all the information from the subprocess directly. Fixes davidhalter#1107.

blueyed · 2018-07-10T10:47:46Z

Thanks!
Rebased.
I would still like to rename Environment.path to Environment.prefix. We could keep path as a (deprecated) alias maybe then?
This could also be done in a separate PR though.

I have tried to implement restarting in case `g:deoplete#sources#jedi#python_path` gets changed, and figured that it is way easier when using only Jedi. There are likely previous features missing, and it needs cleaning up / deleting still. It uses the Jedi branch to improve handling of Environments: davidhalter/jedi#1108

davidhalter · 2018-07-11T11:14:41Z

I agree we should do that. I however have no idea why the tests still fail :-/ I cannot reproduce it locally anymore. I'll try to debug it on travis with debug mode.

davidhalter · 2018-07-11T18:05:11Z

Ok. I can actually reproduce it, but only like this: pytest test_api/test_environment.py test_evaluate/test_sys_path.py. If I just use test_sys_path.py, the test passes. I'm pretty sure it has to do something with caching, but I'm not yet sure why this didn't appear before.

blueyed · 2018-07-12T01:24:58Z

It is caused by test_working_venv running before:

jedi/test/test_api/test_environment.py

Lines 108 to 110 in 71f62c2

    
           def test_working_venv(venv_path): 
        
               with set_environment_variable('VIRTUAL_ENV', venv_path): 
        
                   assert get_default_environment().path == venv_path

.

blueyed · 2018-07-12T01:32:58Z

This fixes it:

diff --git i/jedi/api/environment.py w/jedi/api/environment.py
index 06eb90ec..f6f47511 100644
--- i/jedi/api/environment.py
+++ w/jedi/api/environment.py
@@ -116,6 +116,12 @@ def get_sys_path(self):
         # site.py gets executed.
         return self._get_subprocess().get_sys_path()
 
+    def __del__(self):
+        try:
+            self._subprocess.kill()
+        except AttributeError:
+            pass
+
 
 class SameEnvironment(Environment):
     def __init__(self):

The AttributeError might come from AttributeError: 'SameEnvironment' object has no attribute '_subprocess'.

It makes sense in general I would say, but there is likely still an issue here that needs to be tracked down.

blueyed · 2018-07-12T01:41:13Z

The problem is that create_environment picks up the previous subprocess.
Environment and get_subprocess should probably have an arg to use a new instance (and deleting the existing one).
I would assume that if I call create_environment it would be created after all, and not re-used.

create_environment is meant to be changed anyway (#1155).

I will push the __del__ patch for now.

This kills the subprocess when the Environment instance is about to be destroyed. This works around an internal test failure [1]. 1: davidhalter#1108 (comment)

blueyed · 2018-07-12T01:59:50Z

Only a temporary failure on Travis: https://travis-ci.org/davidhalter/jedi/jobs/402926601 - cannot restart it myself.

davidhalter · 2018-07-12T19:20:21Z

jedi/api/environment.py

@@ -32,7 +32,8 @@ class InvalidPythonEnvironment(Exception):
 class _BaseEnvironment(object):
    @memoize_method
    def get_grammar(self):
-        version_string = '%s.%s' % (self.version_info.major, self.version_info.minor)


BTW: I didn't mention this previously (now I do), but I think it's ok to have a line length of 100 characters occasionally (PEP8 thinks so too), I think these two examples are more readable if they are longer, especially the second one.

Ok, please add it to the config then.

To which config?

setup.cfg:

[flake8] max-line-length = 100

davidhalter · 2018-07-12T19:22:36Z

Now the tests fail because of appveyor. I have removed the Python 3.3 tests for now. Planning to drop it anyway.

I will try to find a good solution for the __del__ stuff. I realized that this is a bit more complicated than I originally thought.

davidhalter · 2018-07-15T14:36:45Z

Please comment on #1176.

blueyed · 2018-07-15T15:24:13Z

Ok, let's close this in favor of #1176.

I have tried to implement restarting in case `g:deoplete#sources#jedi#python_path` gets changed, and figured that it is way easier when using only Jedi. There are likely previous features missing, and it needs cleaning up / deleting still. It uses the Jedi branch to improve handling of Environments: davidhalter/jedi#1108

blueyed commented May 10, 2018

View reviewed changes

This was referenced May 22, 2018

Latest update doesn't allow for using a different python_interpreter without a virtualenv srusskih/SublimeJEDI#250

Closed

Jedi-vim not working, thinks Jedi is not installed davidhalter/jedi-vim#823

Closed

blueyed mentioned this pull request Jun 23, 2018

API: Environment: create_environment is not deterministic #1155

Closed

blueyed force-pushed the prefix branch from 6fbbeff to 0173872 Compare June 23, 2018 09:34

blueyed mentioned this pull request Jun 23, 2018

[WIP/RFC] Improve selection of environments davidhalter/jedi-vim#836

Open

1 task

davidhalter reviewed Jun 30, 2018

View reviewed changes

blueyed force-pushed the prefix branch from 0173872 to 2bb32e5 Compare June 30, 2018 14:41

blueyed mentioned this pull request Jun 30, 2018

highest_pickle_protocol: return minimum for empty list #1158

Closed

blueyed commented Jun 30, 2018

View reviewed changes

blueyed mentioned this pull request Jun 30, 2018

Environment: improve getting sys.prefix #1107

Closed

micbou mentioned this pull request Jul 3, 2018

Revert using highest possible pickle protocol #1164

Closed

blueyed force-pushed the prefix branch from 2bb32e5 to 9e66253 Compare July 5, 2018 16:49

blueyed mentioned this pull request Jul 8, 2018

[WIP/RFC] Only use Jedi, without worker/client/server/cache deoplete-plugins/deoplete-jedi#184

Closed

4 tasks

Improve Environment

71f62c2

It only takes `executable` and gets all the information from the subprocess directly. Fixes davidhalter#1107.

blueyed force-pushed the prefix branch from f86e5b6 to 71f62c2 Compare July 10, 2018 10:46

Add Environment.__del__

b6248db

This kills the subprocess when the Environment instance is about to be destroyed. This works around an internal test failure [1]. 1: davidhalter#1108 (comment)

flake8 fixes

57dba25

davidhalter reviewed Jul 12, 2018

View reviewed changes

davidhalter mentioned this pull request Jul 15, 2018

Second Try: Environment: improve/fix getting sys.prefix #1176

Merged

blueyed closed this Jul 15, 2018

blueyed deleted the prefix branch July 15, 2018 15:24

Environment: improve/fix getting sys.prefix #1108

Environment: improve/fix getting sys.prefix #1108

Conversation

blueyed commented Apr 27, 2018 • edited Loading

davidhalter commented May 1, 2018

blueyed commented May 2, 2018

davidhalter commented May 2, 2018

blueyed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented May 10, 2018

davidhalter commented May 10, 2018

blueyed commented Jun 23, 2018 • edited Loading

davidhalter commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhalter commented Jun 30, 2018

blueyed commented Jun 30, 2018

blueyed commented Jun 30, 2018

blueyed commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micbou commented Jun 30, 2018

blueyed commented Jun 30, 2018

davidhalter commented Jun 30, 2018

micbou commented Jul 5, 2018

blueyed commented Jul 5, 2018

micbou commented Jul 5, 2018

blueyed commented Jul 8, 2018

blueyed commented Jul 8, 2018

davidhalter commented Jul 9, 2018 • edited Loading

davidhalter commented Jul 9, 2018

davidhalter commented Jul 10, 2018

davidhalter commented Jul 10, 2018

blueyed commented Jul 10, 2018

davidhalter commented Jul 11, 2018 • edited Loading

davidhalter commented Jul 11, 2018

blueyed commented Jul 12, 2018

blueyed commented Jul 12, 2018

blueyed commented Jul 12, 2018

blueyed commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhalter commented Jul 12, 2018 • edited Loading

davidhalter commented Jul 15, 2018

blueyed commented Jul 15, 2018

blueyed commented Apr 27, 2018 •

edited

Loading

blueyed commented Jun 23, 2018 •

edited

Loading

davidhalter commented Jul 9, 2018 •

edited

Loading

davidhalter commented Jul 11, 2018 •

edited

Loading

davidhalter commented Jul 12, 2018 •

edited

Loading