New patch importer #1672

brandonwillard · 2018-08-20T04:53:37Z

These changes implement a new importer using sys.path_hooks and importlib semantics. This is more-or-less a completely different approach from #1671 and, its origin, #1518.

Description

Python 3.x is patched in a way that integrates .hy source files into Python's default importlib machinery. In Python 2.7, a PEP-302 "importer" and "loader" is implemented according to the standard import logic (via pkgutil and later pure-Python imp package code).

In both cases, the entry-point for the loaders is through sys.path_hooks only. As well, the import semantics have been updated all throughout to utilize importlib and follow aspects of PEP-420. This, along with some light patches, should allow for basic use of runpy, py_compile and reload.

Design Choices

In all cases, when .hy files are shadowed by same-prefix .py files, Hy will silently use the .hy files.

Kodiologist · 2018-08-20T17:07:49Z

Can you specify which bugs these changes fix, and add tests for them? (I see you added a test for reload, which is good.) I don't know much about the guts of Python's import system, so it's hard for me to judge how much of an improvement these changes are without tests showing fixed bugs.

brandonwillard · 2018-08-20T17:21:58Z

Kodiologist · 2018-08-20T18:50:51Z

Don't forget keywords (but it's probably best to wait till you have a test for a bug before declaring it fixed).

Otherwise, PEP compliance should be a high priority regardless, no?

Not exactly. I'm not against violating a PEP if by so doing we can fix a bug, add a feature we want, shorten the code, or the like. PEP compliance per se is academic.

brandonwillard · 2018-08-20T19:22:38Z

Ah, yeah, I'll amend that last commit with a keyword to the corresponding issue.

When I consider PEP compliance in this scenario, the concern is based in the reproduction of expected behavior and use of standard APIs for the purpose of automatic/seemless support of existing and new functionality, portability across Python implemenations, etc. Unfortunately, not even CPython makes consistent use of these (e.g. runpy and py_compile do not completely use the existing loaders and are a lot less applicable as a result), so not all those benefits are actually present; however, they might be in later versions.

Kodiologist · 2018-08-20T20:55:03Z

Unfortunately, not even CPython makes consistent use of these (e.g. runpy and py_compile do not completely use the existing loaders and are a lot less applicable as a result), so not all those benefits are actually present

That's the sort of thing I was afraid of. If you can implement these things in passing, that's fine, but don't write a lot of code that CPython can't actually use. Unimplemented standards often end up being ignored or changed.

brandonwillard · 2018-08-21T00:07:16Z

True; fortunately compliance was mostly just a matter of reorganizing existing import and compilation code, and even that was exclusively for Python 2.7.

Otherwise, this approach plugs into Python 3.x's existing importer pipeline in only a dozen lines of code! The rest is interface changes (e.g. __import__ to importlib.import_module).

brandonwillard · 2018-08-22T23:23:28Z

@Kodiologist, any other issues related to the import system that I might've missed?

Regardless, what do we need, from me, in order to move forward?

Kodiologist · 2018-08-22T23:40:49Z

There are issues with require that will probably need additional work, but this PR is probably already ambitious enough, so I wouldn't take them on here if I were you. At any rate, this is a very promising PR. I'll review it soon.

brandonwillard · 2018-08-23T03:55:51Z

The require issues are why I chose to do this first; there could be some overlap.

Kodiologist

1. Could you clean up the commit structure? Rebase out the merge commit, avoid committing code that you only end up correcting in a later commit, separate functional changes from whitespace edits, etc. I'll resume reviewing once you've done this.

2. Don't forget to update NEWS.

3. pytest --ignore tests/test_bin.py seems not to ignore the named tests any more. I don't know why you set pytest==3.2.1 in requirements-travis.txt, but downgrading pytest thus doesn't seem to help.

4. It looks like coexisting __init__.py and __init__.hy work differently from how you intended:

> mkdir testlib
> echo 'print("hi")' > testlib/__init__.py
> echo '(print "hy")' > testlib/__init__.hy
> hy
hy 0.15.0+29.gad5b1da1 using CPython(default) 3.7.0 on Linux
=> (import testlib)
hi
=>

I think the right thing to do in this situation is throw an error. One really never ought to have two files distinguished only by .py vs. .hy (unless perhaps the Python file is a translation of the Hy file).

Kodiologist · 2018-08-23T13:00:20Z

conftest.py

+                return _fspath_pyimport(self, **kwargs)
+            except py.path.local.ImportMismatchError:
+                pkgpath = self.pypkgpath()
+                if pkgpath is not None:


It seems more direct to have if pkgpath is None as the condition, with this part in the else.

Kodiologist · 2018-08-23T13:04:40Z

conftest.py

+
+        _fspath_pyimport = py.path.local.pyimport
+
+        def pyimport_patch_mismatch(self, **kwargs):


This function doesn't seem to need any local variables of pytest_collect_file (except for _fspath_pyimport, which is constant), so it could be defined outside of pytest_collect_file instead of being defined on every call.

Kodiologist · 2018-08-23T13:07:26Z

hy/cmdline.py

-            argv = argv[:mloc+2]
+        if len(argv) > mloc + 2:
+            module_args = argv[mloc + 2:]
+            argv = argv[:mloc + 2]


These sorts of purely aesthetic changes are best done in separate commits from functional changes.

Kodiologist · 2018-08-23T13:09:36Z

hy/cmdline.py


    options = parser.parse_args(argv[1:])

    if options.show_tracebacks:
        global SIMPLE_TRACEBACKS
        SIMPLE_TRACEBACKS = False

-    # reset sys.argv like Python
-    sys.argv = options.args + module_args or [""]
-


Your first commit has an extra commented-out line hanging around here (which isn't visible in GitHub's final diff for the whole PR).

Kodiologist · 2018-08-23T13:10:54Z

hy/cmdline.py

-                x.filename, x.errno, x.strerror), file=sys.stderr)
-            sys.exit(x.errno)
+    rv = 0
+    if len(options.files) == 0 or \


It's better to use parentheses than backslashes.

Kodiologist · 2018-08-23T13:18:11Z

hy/importer.py

+        with io.open(fname, 'r', encoding='utf-8') as f:
+            source = f.read()
+
+        if fname.rpartition('.')[-1] == 'hy':


Why not fname.endswith('.hy')?

Kodiologist · 2018-08-23T13:19:04Z

hy/importer.py

+    if PY3:
+        return code, fname
+    else:
+        return code


This could be written more concisely as return (code, fname) if PY3 else code.

Kodiologist · 2018-08-23T13:27:47Z

hy/cmdline.py


 import astor.code_gen

 import hy
-
+from io import open


I think it's clearer to let open, unqualified, refer to the builtin, not to io.open. Or, use builtins.open or io.open explicitly every time.

Kodiologist · 2018-08-23T13:34:58Z

hy/importer.py

+            if self.file and self.file.closed:
+                mod_type = self.etc[2]
+                if mod_type == HY_SOURCE:
+                    # XXX: Hy source files are always text.


Why the "XXX" in this comment?

Kodiologist · 2018-08-23T13:53:47Z

requirements-travis.txt

@@ -1,4 +1,4 @@
-pytest >= 3.2.1
+pytest==3.2.1


This was a fix for errors observed in Travis caused by seemingly incompatible versions of py and pytest.

Is it one of those bugs that appears on Travis but you can't replicate locally?

It was this one. I think I was able to replicate it locally, but it must've been caused by something in the old PR, because I can't now.

You know, I think I saw that once, but I couldn't reproduce it. I say let Travis use the new version for now and let's try to fix that issue when we get the opportunity.

brandonwillard · 2018-08-23T18:29:30Z

@Kodiologist, thanks for the feedback; I'll try to clean up the Git history today.

I should've clarified the whole .py vs. .hy import choices; they currently differ between Python 3.x and Python 2.7 and Python 2.7 should be the situation detailed in the description. This is simply due to an oversight.

Since the Python 3.x implementation is piggybacking off of the existing import machinery, it will choose .py before .hy (in all same-name, different-suffix cases). Changing the order of suffixes in importlib.machinery.SOURCE_SUFFIXES will alter the import order, though. To make it throw an ImportError in Python 3.x, which I also think is a good response to the situation, another patch is probably needed.

The easiest solution (minimal changes) is to have the importer silently prefer .hy files in all Python versions and cases, ignoring any same-name .py files. In other words, Hy prefers Hy.

Kodiologist · 2018-08-23T19:48:47Z

Hmm, I see. Since Hy didn't already have a deliberate policy with respect to coexisting .py and .hy, you don't have to worry about it for this PR if you don't want to, sure.

brandonwillard · 2018-08-24T19:46:25Z

@Kodiologist, all right, cleaned-up those commits.

Also, I set the import preference to .hy files in all cases and versions; that seems like the most consistent approach that doesn't introduce more patches or violate existing tests/expectations.

Kodiologist

This is fantastic work. See my minor inline comments. There are two more important things I'd like you to look at; then, I think we're ready to go.

The speedup of 734cdcd seems to be gone, and hy.lex.lexer and hy.lex.parser are once again getting run when the user executes hy foo.hy and foo is byte-compiled. Can you rejigger the imports to get this working again?
Can you get the tests passing on Travis with the latest pytest?

Kodiologist · 2018-08-25T19:21:03Z

conftest.py

+def pytest_ignore_collect(path, config):
+    return True if (("py3_only" in path.basename and not PY3) or
+                    ("py35_only" in path.basename and not PY35) or
+                    ("py36_only" in path.basename and not PY36)) else None


You can just append or None to the end instead of using if.

Kodiologist · 2018-08-25T19:30:56Z

hy/importer.py

+
+            for entry in path:
+                if os.path.isfile(entry) and subname == '__main__' \
+                    and entry.endswith('.hy'):


Use parens instead of a backslash.

Kodiologist · 2018-08-25T19:31:41Z

hy/importer.py

+            else:
+                path = [os.path.realpath(self.path)]
+
+            file, file_path, etc = None, None, None


It's best not to use the name of a builtin, like file, for a local variable.

Kodiologist · 2018-08-25T19:49:25Z

tests/importer/test_importer.py

+def test_circular():
+    """Test circular imports by creating a temporary file/module that calls a
+    function that imports itself."""
+    with tempfile.NamedTemporaryFile(suffix='.hy',


Instead of having the test write a temporary file, why not commit this file into tests/resources?

Kodiologist · 2018-08-25T19:55:59Z

NEWS.rst

+Internals
+------------------------------
+* PEP-302 rewrite of the importer system.
+


NEWS is for users, so it doesn't need items for internal changes.

Kodiologist · 2018-08-25T19:56:10Z

NEWS.rst

@@ -12,13 +16,16 @@ New Features
 * Keyword objects (not just literal keywords) can be called, as
  shorthand for `(get obj :key)`, and they accept a default value
  as a second argument.
+* Module reloading works now.


This change is already listed below as a bug fix.

Kodiologist · 2018-08-25T19:57:15Z

NEWS.rst


 Bug Fixes
 ------------------------------
 * Fixed bugs in the handling of unpacking forms in method calls and
  attribute access.
 * Fixed crashes on Windows when calling `hy-repr` on date and time
  objects.
+* Fixed module reloading, circular imports, and `__main__` file


I'd say that each of these fixes is notable enough to get its own item.

brandonwillard · 2018-08-25T23:18:03Z

Regarding the speed-up (or lack thereof), that's probably due to the use of runpy[.run_path] (in cmdline.py), which neither caches nor uses bytecode.

Also, python *.py doesn't appear to cache or use bytecode in either Python 2.7 or 3.6 (per the verbose output messages); however, if you run python -m * in Python 3.6, bytecode is cached and read, while in 2.7 it isn't.

Not sure how we want to handle these cases in Hy, but perhaps some of the latency issues you observed were due to that.

Kodiologist · 2018-08-26T00:01:06Z

In Hy, it's important that hy foo.hy read and write bytecode, because compilation can be arbitrarily slow (because it involves macro-expansion). I'm sorry; I missed the fact that you removed hy {fpath} from test_bin_hy_byte_compile.

brandonwillard · 2018-08-26T00:42:52Z

No problem, it can be easily made to use the cache.

Python 3.x is patched in a way that integrates `.hy` source files into Pythons default `importlib` machinery. In Python 2.7, a PEP-302 "importer" and "loader" is implemented according to the standard `import` logic (via `pkgutil` and later pure-Python `imp` package code). In both cases, the entry-point for the loaders is through `sys.path_hooks` only. As well, the import semantics have been updated all throughout to utilize `importlib` and follow aspects of PEP-420. This, along with some light patches, should allow for basic use of `runpy`, `py_compile` and `reload`. In all cases, if a `.hy` file is shadowed by a `.py`, Hy will silently use `.hy`.

Closes hylang#1466.

Closes hylang#459.

Closes hylang#1457.

Closes hylang#1134.

This test ensures that Hy will load a `.hy` instead of a `.py` counterpart.

brandonwillard · 2018-08-26T05:29:26Z

All right, those changes are in. Also, I added the hy {fpath} tests back to test_bin_hy_byte_compile.

brandonwillard · 2018-08-26T16:43:41Z

It looks like we can get rid of some boilerplate code in HyLoader (e.g. get_source, get_filename, _reopen at ~43 lines) if we adopt the existing imp.PY_SOURCE module type instead of using our own HY_SOURCE.

This change only affects Python 2.7, and it somewhat more closely reflects the patched loader for Python 3.x, since — in the latter case — .hy is added as an importlib.machinery.SOURCE_SUFFIXES.

The naming alone (i.e. PY_SOURCE) is a little less welcoming of this change (Hy source is definitely not Py source), but, with the rest of the custom loader overrides, it really is just naming and nothing more.

Kodiologist · 2018-08-26T17:01:39Z

Sounds good. Try it out.

brandonwillard · 2018-08-26T17:32:32Z

hy/importer.py

-            if self.file and self.file.closed:
-                mod_type = self.etc[2]
-                if mod_type == HY_SOURCE:
-                    self.file = io.open(self.filename, 'rU', encoding='utf-8')


Not so sure about removing this override; the default ImpLoader._reopen simply does open(self.filename, 'rU'), which won't necessarily use the unicode codec (I think).

Hmm, yeah, that could lead to trouble. You could try writing a test with a source file that has non-ASCII characters in it.

Or is this already implicitly tested by all the other native tests that have non-ASCII characters?

It is, but most likely not in a situation where ImpLoader._reopen is called.

Just re-added a custom _reopen that guarantees unicode decoding.

brandonwillard force-pushed the new-patch-importer branch from 307d7d3 to 5450652 Compare August 20, 2018 15:34

Kodiologist reviewed Aug 23, 2018

View reviewed changes

brandonwillard force-pushed the new-patch-importer branch from ad5b1da to 9a569d7 Compare August 24, 2018 19:36

brandonwillard force-pushed the new-patch-importer branch 2 times, most recently from ce6a21f to f065e8d Compare August 25, 2018 00:01

Kodiologist reviewed Aug 25, 2018

View reviewed changes

brandonwillard added 3 commits August 25, 2018 22:50

Add module reloading tests

e9e7171

Add tests for importing __main__.hy files

4839aca

Closes hylang#1466.

brandonwillard force-pushed the new-patch-importer branch 2 times, most recently from 05bb5ec to 343d48f Compare August 26, 2018 04:44

brandonwillard added 4 commits August 26, 2018 00:17

Add Python cmdline bytecode option and set sys.executable

c022abc

Closes hylang#459.

Use Python cmdline file-relative sys.path

cbaba4a

Closes hylang#1457.

Add test for circular imports

1da2941

Closes hylang#1134.

Add test for shadowed-basename imports

bbc66d0

This test ensures that Hy will load a `.hy` instead of a `.py` counterpart.

brandonwillard added 2 commits August 26, 2018 00:27

Update NEWS

b12fd33

Fix pytest hook so that ignore works consistently

32033b0

brandonwillard force-pushed the new-patch-importer branch from 343d48f to 32033b0 Compare August 26, 2018 05:27

Kodiologist approved these changes Aug 26, 2018

View reviewed changes

brandonwillard commented Aug 26, 2018

View reviewed changes

Make Hy a Python-source module type

2ea1e8e

brandonwillard force-pushed the new-patch-importer branch from d1236bc to 2ea1e8e Compare August 26, 2018 18:21

Add a test for module docstrings

5d325a5

brandonwillard force-pushed the new-patch-importer branch from a785985 to 5d325a5 Compare August 27, 2018 06:02

brandonwillard mentioned this pull request Sep 3, 2018

Update version requirements and setuptools testing brandonwillard/loghyc#10

Merged

Kodiologist merged commit 4af87dc into hylang:master Sep 3, 2018

brandonwillard deleted the new-patch-importer branch September 3, 2018 23:59

This was referenced Sep 4, 2018

Hy will treat a Hy file as Python if it doesn't have a ".hy" file extension #1677

Closed

Enable Hy code in the debugger #1680

Closed

Fix missing import in doc macro expansion #1679

Merged

This was referenced Oct 7, 2018

Import failures prevent from re-trying #1523

Closed

Loader doesn't work correctly #1514

Closed

Kodiologist mentioned this pull request Feb 28, 2019

A New importer #1518

Closed

9 tasks


		_fspath_pyimport = py.path.local.pyimport

		def pyimport_patch_mismatch(self, **kwargs):

New patch importer #1672

New patch importer #1672

Conversation

brandonwillard commented Aug 20, 2018 • edited Loading

Description

Design Choices

Kodiologist commented Aug 20, 2018

brandonwillard commented Aug 20, 2018 • edited Loading

Kodiologist commented Aug 20, 2018

brandonwillard commented Aug 20, 2018 • edited Loading

Kodiologist commented Aug 20, 2018

brandonwillard commented Aug 21, 2018

brandonwillard commented Aug 22, 2018

Kodiologist commented Aug 22, 2018

brandonwillard commented Aug 23, 2018

Kodiologist 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

Kodiologist Aug 23, 2018 • edited Loading

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

brandonwillard commented Aug 23, 2018 • edited Loading

Kodiologist commented Aug 23, 2018

brandonwillard commented Aug 24, 2018

Kodiologist left a comment • edited Loading

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

brandonwillard commented Aug 25, 2018 • edited Loading

Kodiologist commented Aug 26, 2018

brandonwillard commented Aug 26, 2018

brandonwillard commented Aug 26, 2018

brandonwillard commented Aug 26, 2018

Kodiologist commented Aug 26, 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

brandonwillard commented Aug 20, 2018 •

edited

Loading

brandonwillard commented Aug 20, 2018 •

edited

Loading

brandonwillard commented Aug 20, 2018 •

edited

Loading

Kodiologist Aug 23, 2018 •

edited

Loading

brandonwillard commented Aug 23, 2018 •

edited

Loading

Kodiologist left a comment •

edited

Loading

brandonwillard commented Aug 25, 2018 •

edited

Loading