Refactor interpreter evaluation #137

Kyle-Verhoog · 2021-05-15T19:39:39Z

Before, interpreter evaluation was done at run-time when the interpreter
was first used.

With this patch interpreters are looked up when the environments are
generated.

Also more precise interpreter specification is possible. Before,
specifying "3" in a riotfile would result in a "python3" executable
being looked up. So riot would fail if Python was installed as "python"
and no "python3" executable was available on the path.

Now the path is searched for python executables which are then
referenced when resolving the python to use for an environment.

Before, interpreter evaluation was done at run-time when the interpreter was first used. With this patch interpreters are looked up when the environments are generated. Also, more precise interpreter specification is possible. Before, specifying "3" in a riotfile would result in a "python3" executable being looked up. So riot would fail if Python was installed as "python" and no "python3" executable was available on the path. Now interpreters can be specified with a file path, executable name or a version (which will be looked up on the PATH).

jd

I like the overall direction :)

jd · 2021-05-17T12:36:36Z

.github/workflows/main.yml

@@ -1,9 +1,6 @@
 name: CI
 on:
-  pull_request:


Sounds unrelated.

jd · 2021-05-17T12:37:11Z

riot/cli.py

+        interp = Interpreter.parse(value)
+        return interp


Suggested change

interp = Interpreter.parse(value)

return interp

return Interpreter.parse(value)

but ok 😁

jd · 2021-05-17T12:42:11Z

riot/riot.py

+    def __hash__(self) -> int:
+        """Return the hash of this interpreter."""
+        return hash(self._path)


Don't do this, just pass hash=False to _version L72.

jd · 2021-05-17T12:42:30Z

riot/riot.py

+    def __repr__(self) -> str:
+        """Return the repr containing the path and version of this interpreter."""
+        return f"{self.__class__.__name__}('{self.version}', '{self._path}')"


Is that really better than dataclass generated method?

jd · 2021-05-17T12:42:44Z

riot/riot.py

    def __str__(self) -> str:
-        """Return the path of the interpreter executable."""
+        """Return the repr of this interpreter."""
        return repr(self)


ditto, not worth it I guess with dataclass doing the job.

jd · 2021-05-17T12:45:22Z

riot/riot.py

+        FIXME: unfortunately Version will set undefined segments to 0 so
+        we cannot distinguish between "3.0.0" and "3".


I don't think it's a problem, in general, and especially in our case.

jd · 2021-05-17T12:46:10Z

riot/riot.py

+        interp_paths: t.List[str] = []
+        # packaging says that the regular expression needs to be compiled
+        # with the following flags
+        expr = re.compile(f"python{VERSION_PATTERN}$", re.IGNORECASE | re.VERBOSE)


define this has a class variable to compile once and for all

jd · 2021-05-17T12:48:38Z

riot/riot.py

+            if not os.path.isdir(d):
+                continue
+            for f in os.listdir(d):


As a rule of thumb, never do that. The pattern of calling stat before doing an I/O is at best unoptimized, at worst a security issue.

You should always do a single file operation and catch errors, like this:

Suggested change

if not os.path.isdir(d):

continue

for f in os.listdir(d):

try:

for f in os.listdir(d):

[...]

except NotADirectoryError:

continue

jd · 2021-05-17T12:55:39Z

riot/riot.py

+        interps = [i for i in cls.find() if i.matches(v)]
+        if len(interps):
+            return interps[0]
+        raise FileNotFoundError("No interpreter matching %r", v)


There's no need to build the entire list. If it huges, it'll take hours for nothing.
You could use first which I like for this: https://pypi.org/project/first/
Otherwise:

Suggested change

interps = [i for i in cls.find() if i.matches(v)]

if len(interps):

return interps[0]

raise FileNotFoundError("No interpreter matching %r", v)

for interp in cls.find():

if interp.matches(v):

return interp

raise FileNotFoundError("No interpreter matching %r", v)

jd · 2021-05-17T12:56:34Z

riot/riot.py

+        except InvalidVersion as e:
+            raise FileNotFoundError("No interpreter found for %r" % s) from e


I wouldn't change that exception, it does not seem fair.

A bit more clear that the "version" is not necessarily a version.

CmdFailure was overloaded to handle two different error cases which was confusing.

Print out the venv explicitly saying that the specified interpreter is missing.

brettlangdon · 2021-05-23T13:57:11Z

setup.py

@@ -28,6 +28,7 @@
    install_requires=[
        "dataclasses; python_version<'3.7'",
        "click>=7,<8",
+        "packaging",


is there a min version we need?

brettlangdon · 2021-05-23T14:00:16Z

riot/riot.py

+    def __post_init__(self):
+        """Get the version of the interpreter."""
+        output = subprocess.check_output(
+            [self._path, "--version"], stderr=subprocess.STDOUT


you might want to rebase on master we made some changes to how we get version from Python to alleviate the parsing of --version output.

riot/riot/riot.py

Lines 81 to 91 in 6d064ca

def version(self) -> str:

path = self.path()

output = subprocess.check_output(

[

path,

"-c",

'import sys; print("%s.%s.%s" % (sys.version_info.major, sys.version_info.minor, sys.version_info.micro))',

],

)

return output.decode().strip()

brettlangdon · 2021-05-23T14:05:55Z

riot/riot.py

+    def _matches(v1: Version, v2: Version) -> bool:
+        """Return if v2 matches v1.


The better approach is to have a custom Version class that inherits from packaging.version.Version and which defines a custom Version._key.

https://github.com/pypa/packaging/blob/47d40f640fddb7c97b01315419b6a1421d2dedbb/packaging/version.py#L292-L299

Seems like we want to compare versions but ignoring pre/post/dev/local ?

which if that is the case, then a custom key, and then relying on the built-in comparisons to Version should work.

brettlangdon · 2021-05-23T14:10:29Z

riot/riot.py

+        """Return whether the given version matches the version of the interpreter."""
+        return self._matches(self.version, version)
+
+    def path(self) -> str:


Suggested change

def path(self) -> str:

@property

def path(self) -> str:

?

brettlangdon · 2021-05-23T14:12:28Z

riot/riot.py

@@ -233,20 +346,26 @@ class VenvInstance:
    env: t.Tuple[t.Tuple[str, str]]
    name: t.Optional[str]
    pkgs: t.Tuple[t.Tuple[str, str]]
-    py: Interpreter
+    interpreter_hint: str
+    """A null interpreter means that it was not found."""


Suggested change

"""A null interpreter means that it was not found."""

# A null interpreter means that it was not found.

?

brettlangdon · 2021-05-23T14:14:40Z

riot/riot.py

+                pkgs: t.Dict[str, str] = {
+                    name: version for name, version in inst.pkgs if version is not None
+                }


I found transforming/relocating this and pkg_str into VenvInstance much nicer to work with.

brettlangdon · 2021-05-23T14:16:45Z

riot/riot.py

+                pkgs_str = " ".join(
+                    f"'{get_pep_dep(name, version)}'" for name, version in inst.pkgs
+                )


relocating this to VenvInstance will definitely be nice since it seems like we do this transformation in at least 3 places.

brettlangdon · 2021-05-23T14:17:16Z

riot/riot.py

+                )
+                env_str = env_to_str(inst.env)
+                click.echo(
+                    f"{inst.name} {env_str} <Interpreter missing for '{inst.interpreter_hint}'> {pkgs_str}"


should the str/repr output of Interpreter just state whether it exists or not?

brettlangdon · 2021-05-23T14:18:02Z

riot/riot.py

            pkgs_str = " ".join(
                f"'{get_pep_dep(name, version)}'" for name, version in inst.pkgs
            )
            env_str = env_to_str(inst.env)
-            py_str = f"Python {inst.py}"
-            click.echo(f"{inst.name} {env_str} {py_str} {pkgs_str}")
+            click.echo(f"{inst.name} {env_str} {inst.interpreter} {pkgs_str}")


should we make VenvInstance.__str__ print what we expect here?

or else a helper method would help since we have this format in at least 2 places

brettlangdon · 2021-05-23T14:19:10Z

riot/riot.py

+
+        for inst in self.venv.instances(pattern=pattern):
+            if inst.interpreter is None:
+                continue


raise?

wondering how we can end up in this state other than misconfiguration?

Kyle-Verhoog requested a review from a team as a code owner May 15, 2021 19:39

Kyle-Verhoog force-pushed the interp-lookup branch 5 times, most recently from 1252184 to a6fa6a2 Compare May 16, 2021 05:37

jd suggested changes May 17, 2021

View reviewed changes

Kyle-Verhoog added 4 commits May 20, 2021 21:49

Inline return

40a8de4

Atomic directory read

36e1d19

Don't wrap invalid version errors on resolve

68ef1cd

interpreter_version -> interpreter_hint

63af679

A bit more clear that the "version" is not necessarily a version.

Kyle-Verhoog mentioned this pull request May 21, 2021

Add test helpers, rewrite integration tests #100

Merged

Kyle-Verhoog added 3 commits May 21, 2021 21:46

Refactor run with RunFailure

65a1b55

CmdFailure was overloaded to handle two different error cases which was confusing.

Fix list with missing interpreters

7f4eca3

Print out the venv explicitly saying that the specified interpreter is missing.

Fix generate venvs

6724866

Kyle-Verhoog force-pushed the interp-lookup branch from a6fa6a2 to 6724866 Compare May 22, 2021 02:25

brettlangdon reviewed May 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor interpreter evaluation #137

Refactor interpreter evaluation #137

Kyle-Verhoog commented May 15, 2021 •

edited

jd left a comment

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

jd May 17, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

brettlangdon May 23, 2021

	interp = Interpreter.parse(value)
	return interp
	return Interpreter.parse(value)

		FIXME: unfortunately Version will set undefined segments to 0 so
		we cannot distinguish between "3.0.0" and "3".

		except InvalidVersion as e:
		raise FileNotFoundError("No interpreter found for %r" % s) from e

	def version(self) -> str:
	path = self.path()

	output = subprocess.check_output(
	[
	path,
	"-c",
	'import sys; print("%s.%s.%s" % (sys.version_info.major, sys.version_info.minor, sys.version_info.micro))',
	],
	)
	return output.decode().strip()

		def _matches(v1: Version, v2: Version) -> bool:
		"""Return if v2 matches v1.

	"""A null interpreter means that it was not found."""
	# A null interpreter means that it was not found.

Refactor interpreter evaluation #137

Are you sure you want to change the base?

Refactor interpreter evaluation #137

Conversation

Kyle-Verhoog commented May 15, 2021 • edited

jd 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Verhoog commented May 15, 2021 •

edited