Skip to content

BUG: import full module path in npy_load_module - #8148

Merged
charris merged 1 commit into
numpy:masterfrom
jjhelmus:npy_load_fix
Oct 13, 2016
Merged

BUG: import full module path in npy_load_module#8148
charris merged 1 commit into
numpy:masterfrom
jjhelmus:npy_load_fix

Conversation

@jjhelmus

@jjhelmus jjhelmus commented Oct 12, 2016

Copy link
Copy Markdown
Contributor

Use the full module path when importing importlib.machinery for use in the
npy_load_module function. Just importing importlib is not sufficient in certain
cases, for example Python 3.4.

closes #8147

@jjhelmus
BUG: import full module path in npy_load_module
5d0ce36 
Use the full module path when importing importlib.machinery for use in the
npy_load_module function. Just importing importlib is not sufficient in certain
cases, for example Python 3.4.

closes numpy#8147
@jjhelmus jjhelmus mentioned this pull request Oct 12, 2016
Merged
@njsmith

njsmith commented Oct 12, 2016

Copy link
Copy Markdown
Member

Change looks good but can we get a test?

@jjhelmus

jjhelmus commented Oct 12, 2016

Copy link
Copy Markdown
Contributor Author

Was thinking about the following as a test:

def test_npy_load_module_py34():
    with tempdir(prefix="numpy_test_compat_") as folder:
        filename = join(folder, 'example.py')
        with open(filename, 'w') as f:
            f.write("magic_number = 42")
        example = npy_load_module('example', filename)
        assert_(example.magic_number == 42)

The issue is that this passes with or without the proposed fix. Outside of the test suite the assert will fail without the fix but inside the test suite it always passes.

@jjhelmus

jjhelmus commented Oct 12, 2016

Copy link
Copy Markdown
Contributor Author

Fun, machinery is not in the importlib module namespace if imported alone, but it is after nose is imported:

Python 3.4.5 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:47:57)
[GCC 4.2.1 Compatible Apple LLVM 4.2 (clang-425.0.28)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import importlib
>>> dir(importlib)
['_RELOADING', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__import__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '_bootstrap', '_imp', '_r_long', '_w_long', 'find_loader', 'import_module', 'invalidate_caches', 'reload', 'sys', 'types', 'warnings']
>>> import nose
>>> dir(importlib)
['_RELOADING', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__import__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '_bootstrap', '_imp', '_r_long', '_w_long', 'find_loader', 'import_module', 'invalidate_caches', 'machinery', 'reload', 'sys', 'types', 'util', 'warnings']

I'm at a loss on how write a test that changes from pass to fail with the proposed modification.

@njsmith

njsmith commented Oct 12, 2016

Copy link
Copy Markdown
Member

Ugh, wow.

Hail mary: subprocess.run([sys.executable, "-c", "import numpy; numpy.npy_load_module(..."])?

Or if it's not doable we can skip it.

@rgommers rgommers added this to the 1.12.0 release milestone Oct 13, 2016
@charris
charris merged commit e5d7653 into numpy:master Oct 13, 2016
@charris

charris commented Oct 13, 2016

Copy link
Copy Markdown
Member

@njsmith I'm going to put this in, figuring out how to test it looks to be more trouble than it is worth.

@charris

charris commented Oct 13, 2016

Copy link
Copy Markdown
Member

Thanks @jjhelmus .

@homu homu mentioned this pull request Oct 13, 2016
Closed
@jakirkham

jakirkham commented Oct 13, 2016

Copy link
Copy Markdown
Contributor

Would this be backported to 1.11?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

03 - Maintenance component: numpy.distutils

Projects

None yet

Milestone

1.12.0 release

Development

Successfully merging this pull request may close these issues.

Configuration.add_subpackage fails on Python 3.4

5 participants

@jjhelmus @njsmith @charris @jakirkham @rgommers