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

Allowed importing pyx files from ZIP archives #1485

Merged
merged 2 commits into from Feb 11, 2017

Conversation

superbobry
Copy link
Contributor

Python allows to import and run modules from ZIP archives. I thought it would be useful if pyximport handled this case!

Here's a quick demo:

$ cat _speedups.pyx
def main():
    print("Hello, world!")
$ cat __main__.py
import pyximport
pyximport.install()

import _speedups
_speedups.main()
$ zip demo.zip _speedups.pyx __main__.py
  adding: _speedups.pyx (stored 0%)
  adding: __main__.py (deflated 29%)
$ PYTHONPATH=. python3 demo.zip
...
8 warnings generated.
Hello, world!

join_path = os.path.join
is_file = os.path.isfile
is_abs = os.path.isabs
abspath = os.path.abspath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could move these into imports instead (I often import os.path.join as join_path), but I wouldn't remove them entirely. They are just too verbose when spelled out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boundary between "too verbose" and "not too verbose" is often subtle. To me the extra characters os. or os.path add are acceptable. Moreover, the rest of the file uses os.path directly without any aliases, so I think it is best to make this bit consistent.

import test_zip_module
assert test_zip_module.x == 42
finally:
os.remove(zip_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the change to sys.path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be explicit about removing the exact path that was added. The import executes a lot of code, there is not guarantee that sys.path still looks the same afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

else:
assert False, "test_zip_module already exists"

fd, zip_path = tempfile.mkstemp(suffix=".zip")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use NamedTemporaryFile here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but this would mean extra work w/o any added benefit:

with tempfile.NamedTemporaryFile(delete=False) as handle:
    with ZipFile(zip_path, "w") as zf:
        zf.writestr("test_zip_module.pyx", b"x = 42")

sys.path.insert(0, handle.path)
import test_zip_module

try:
    assert test_zip_module.x == 42
finally:
    os.remove(handle.path)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the NTF would do the cleanup for you. No need for the try-finally in the end. (Besides, "handle" is not a good name for a variable that holds a file-like object.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will use NTF. Could you suggest a better name for "handle"?

Note, that we would still need finally to remove the entry from sys.path.

Update: turns out we cannot use NTF after all. Here's an excerpt from the docs:

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

zi = zipimporter(path)
data = zi.get_data(pyx_module_name)
except (ZipImportError, IOError):
pyx_module_path = os.path.join(path, pyx_module_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more efficient to look up the path first, and only failing that, try to zip-import it? This doesn't seem like a feature that all that many people would use, especially not during development.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on this, please? The current implementation only uses ZIP importer if file-based importer failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not if the shared library does not exist yet.

I just read the source and the zipimporter is actually quite efficient here. The first thing it does is to check if the path it received is a file. Still, I don't see why we shouldn't check if path points to a directory first (because that's really the only case we can handle) and only failing that, fall back to trying the zipimporter. No need to involve the zipimporter for the obvious and expected "it's a directory but it does not contain the file we search" case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a new patch. Now zipimport is only called if path is a file.

@scoder
Copy link
Contributor

scoder commented Feb 11, 2017

Looks good. Thanks!

@scoder scoder merged commit 4038908 into cython:master Feb 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants