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

Fix find_current_module so that it can work in application bundles #8845

Merged
merged 7 commits into from Jul 10, 2019

Conversation

Projects
None yet
6 participants
@astrofrog
Copy link
Member

commented Jun 14, 2019

As part of work on making it possible to use astropy in application bundles (#8795) I had to re-use a fix @embray originally wrote in #960 but since that PR never got merged, I thought I'd open a dedicated PR for this fix with a test. Essentially the goal here is to make sure that find_current_module works even if inspect.getmodule doesn't work, which can happen if astropy is packaged up inside a bundle created by e.g. pyinstaller or py2app.

@astrofrog astrofrog added the utils label Jun 14, 2019

@astrofrog astrofrog added this to the v2.0.14 milestone Jun 14, 2019

@astrofrog astrofrog added the Bug label Jun 14, 2019

@pllim

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Do we really need to backport this that far to 2.x series? People still trying to bundle for Python 2? If not, it is easier to backport only to 3.x.

@pllim

pllim approved these changes Jun 14, 2019

Copy link
Member

left a comment

I like the extensive comments, but I do have a question of milestone. Otherwise, LGTM.

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

No strong feelings about milestone, but it's possible people will still use LTS with e.g. pyinstaller.

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Ok, so there is a failure with one of the modeling doctests:

______________ [doctest] astropy.modeling.core._ModelMeta.rename _______________
201 
202         Creates a copy of this model class with a new name.
203 
204         The new class is technically a subclass of the original class, so that
205         instance and type checks will still work.  For example::
206 
207             >>> from astropy.modeling.models import Rotation2D
208             >>> SkyRotation = Rotation2D.rename('SkyRotation')
209             >>> SkyRotation
Differences (unified diff with -expected +actual):
    @@ -1,3 +1,3 @@
    -<class '__main__.SkyRotation'>
    +<class 'astropy.modeling.core.astropy.modeling.core.SkyRotation'>
     Name: SkyRotation (Rotation2D)
     Inputs: ('x', 'y')
/tmp/astropy-test-f_0hhe45/lib/python3.7/site-packages/astropy/modeling/core.py:209: DocTestFailure

What do we think is the correct behavior here? (@nden?)

@bsipocz bsipocz modified the milestones: v2.0.14, v2.0.15 Jun 15, 2019

@astrofrog astrofrog requested a review from nden Jun 28, 2019

@nden

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I like the new output because it's clear the class comes from the modeling package.
It's always going to be in the modeling.core module so I think this is also good.
I can't think of any disadvantages of having this path to the new class.

@nden

nden approved these changes Jun 28, 2019

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Alternatively we can always use an ellipsis for that line.

@pllim

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Change log also needs updating.

@astrofrog astrofrog force-pushed the astrofrog:find-current-module-bundle branch from f95d17a to 35d0fae Jun 28, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

Looks like this still needs some work on Windows. I'll investigate next week.

@saimn

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

__main__.SkyRotation makes more sense to me, this is a new class so it should inherit the module (or here __main__) where it is defined.
Also the duplicated module name in astropy.modeling.core.astropy.modeling.core.SkyRotation is a bit weird.

@astrofrog astrofrog force-pushed the astrofrog:find-current-module-bundle branch from 35d0fae to ee4dfcb Jul 3, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@saimn - good catch about the duplicate path in the module name, not sure why I didn't see that. This was a real bug unrelated to the original intent of this PR, and is now fixed.

I think that for the doctest, the path to the class is correct since the doctest is in fact inside astropy.modeling.core - but I've now added a more extensive test that ensures that if a user runs a script, the path to the class will include __main__, and indeed it does work correctly.

new_cls.__qualname__ = name
else:
new_cls.__qualname__ = '{0}.{1}'.format(modname, name)
new_cls.__qualname__ = name

This comment has been minimized.

Copy link
@astrofrog

astrofrog Jul 3, 2019

Author Member

This was a bug - __qualname__ should not include the module name (see PEP 3155)

@astrofrog astrofrog requested a review from nden Jul 3, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@nden - can you review this updated version?

@astrofrog astrofrog requested a review from saimn Jul 3, 2019

@bsipocz bsipocz requested a review from pllim Jul 3, 2019

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

(I've asked the re-review from @pllim, too in the hope of not having the green ticks on the right hand side would also mean that the "Approved" status will also disappear when looking at the PR list. Apparently that's not the case 😞 )

@saimn

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I think that for the doctest, the path to the class is correct since the doctest is in fact inside astropy.modeling.core

Yes indeed, though it depends on how you execute the doctests (python -m doctest or import doctest; doctest.testmod() will give different outputs). With pytest and the current directory structure, pytest imports the module from the package so I think that the current output (with your PR) is good.

@astrofrog astrofrog changed the title Find find_current_module so that it can work in application bundles Fix find_current_module so that it can work in application bundles Jul 6, 2019

@pllim

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Might need a rebase? The test failure doesn't make much sense:

ERROR: Python >=3.6 is required by astropy
@pllim

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Looks like this still needs some work on Windows.

@astrofrog , is the Windows work all done?

@nden

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@astrofrog @saimn What do you think is sensible in the case when a model is defined in another package, for example:

from gwcs.selector import LabelMapper
lm = LabelMapper.rename('LLL')

With the current PR and current master

lm                                                                                                                                            
Out[3]: 
<class '__main__.LLL'>
Name: LLL (LabelMapper)
Inputs: <property object at 0x7f42e517d048>
Outputs: ('label',)

I don't have a use case for this method but (perhaps naively) I would expect to see a reference to the gwcs.selector module.

If I change the rename method to get the module name by

new_cls = type(name, (cls,), {})
mod = inspect.getmodule(cls)

I get instead

Out[3]: 
<class 'gwcs.selector.LLL'>
Name: LLL (LabelMapper)
Inputs: <property object at 0x7f78cb028ef8>
Outputs: ('label',)

And for the doctest example

Out[6]: 
<class 'astropy.modeling.rotations.SkyRotation'>
Name: SkyRotation (Rotation2D)
Inputs: ('x', 'y')
Outputs: ('x', 'y')
Fittable parameters: ('angle',)

That said I don't know if this would prevent it from running in a bundle (the original issue being solved).
I'm mostly trying to understand the issue.

@astrofrog astrofrog force-pushed the astrofrog:find-current-module-bundle branch from 08d029e to 5c1f10a Jul 8, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@nden - yes I think that for models defined in other packages we should indeed do better than just __main__. Since that occurs in both master and this PR, I suggest that might be best dealt with in a separate issue/PR though?

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Oops I force pushed an older version of the branch. I'll have to see if I can recover the original version. I'm traveling so this might have to wait until after, as I did this on my desktop.

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@astrofrog - I guess using the hash above you might be able to recover it from your fork?

embray and others added some commits Dec 3, 2013

This implements an alternate solution to fixing find_current_module w…
…hen importing from a zip file. I think this is a little simpler than my previous solution and appears to work just as well. It avoids relying at all on source filenames in code objects, which I've learned are misused in all sorts of inconsistent ways.
Fix bug with full name for class when renaming - this was due to the …
…fact that __qualname__ should not include the module name. Also expanded test for the path to the class when renaming.

@astrofrog astrofrog force-pushed the astrofrog:find-current-module-bundle branch from 5c1f10a to b555376 Jul 9, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Ok so this wasn't super easy as there's no way to actually just fetch the commits directly - instead I had to download each original commit as a patch and apply it with git am, e.g.

wget https://github.com/astropy/astropy/commit/08d029e6b85be197590d43811e59acae7ea06d70.patch
git am 08d029e6b85be197590d43811e59acae7ea06d70.patch

Bottom line: this PR should now be back to the latest version.

@pllim

pllim approved these changes Jul 9, 2019

Copy link
Member

left a comment

I am okay with this as long as @nden is okay with the modeling stuff when she re-review this PR.

@nden

nden approved these changes Jul 9, 2019

@pllim

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@astrofrog , just to confirm, do you still have to "check something on Windows" or not?

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

@nden - no, I did it and fixed the issue, AppVeyor is passing :)

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

I'll go ahead and merge since this was approved!

@astrofrog astrofrog merged commit 1d99862 into astropy:master Jul 10, 2019

13 of 15 checks passed

codecov/patch 85% of diff hit (target 86.58%)
Details
codecov/project 86.57% (-0.01%) compared to c0b29d8
Details
astropy-bot:changelog Changelog entry consistent with milestone
astropy-bot:milestone This pull request has a milestone set.
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl302 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl310 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

I get all kind of errors with this on LTS, so given that no strong preference was shown above, would incline not to backport. One example (39 failures in total):

_______________________________________ test_zero_degree_polynomial[Chebyshev2D] _______________________________________

cls = <class 'astropy.modeling.polynomial.Chebyshev2D'>
Name: Chebyshev2D (OrthoPolynomialBase)
Inputs: ('x', 'y')
Outputs: ('z',)
Fittable parameters: <property object at 0x12abfa778>

    @pytest.mark.parametrize('cls', (Polynomial1D, Chebyshev1D, Legendre1D,
                                     Polynomial2D, Chebyshev2D, Legendre2D))
    def test_zero_degree_polynomial(cls):
        """
        A few tests that degree=0 polynomials are correctly evaluated and
        fitted.
    
        Regression test for https://github.com/astropy/astropy/pull/3589
        """
    
        if cls.n_inputs == 1:  # Test 1D polynomials
            p1 = cls(degree=0, c0=1)
            assert p1(0) == 1
            assert np.all(p1(np.zeros(5)) == np.ones(5))
    
            x = np.linspace(0, 1, 100)
            # Add a little noise along a straight line
            y = 1 + np.random.uniform(0, 0.1, len(x))
    
            p1_init = cls(degree=0)
            fitter = fitting.LinearLSQFitter()
            p1_fit = fitter(p1_init, x, y)
    
            # The fit won't be exact of course, but it should get close to within
            # 1%
            assert_allclose(p1_fit.c0, 1, atol=0.10)
        elif cls.n_inputs == 2:  # Test 2D polynomials
            if issubclass(cls, OrthoPolynomialBase):
                p2 = cls(x_degree=0, y_degree=0, c0_0=1)
            else:
                p2 = cls(degree=0, c0_0=1)
            assert p2(0, 0) == 1
            assert np.all(p2(np.zeros(5), np.zeros(5)) == np.ones(5))
    
            y, x = np.mgrid[0:1:100j, 0:1:100j]
            z = (1 + np.random.uniform(0, 0.1, x.size)).reshape(100, 100)
    
            if issubclass(cls, OrthoPolynomialBase):
                p2_init = cls(x_degree=0, y_degree=0)
            else:
                p2_init = cls(degree=0)
            fitter = fitting.LinearLSQFitter()
            p2_fit = fitter(p2_init, x, y, z)
    
>           assert_allclose(p2_fit.c0_0, 1, atol=0.10)
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0.1
E           
E           Mismatch: 100%
E           Max absolute difference: 1.
E           Max relative difference: 1.
E            x: array(0.)
E            y: array(1)

astropy/modeling/tests/test_polynomial.py:362: AssertionError
------------------------------------------------- Captured stderr call -------------------------------------------------
Python(78280,0x7fff971c3380) malloc: *** mach_vm_map(size=18446744071772295168) failed (error code=3)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug
init_dgelsd failed init
__________________________

@bsipocz bsipocz modified the milestones: v2.0.15, v3.2.2 Jul 15, 2019

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Oh fun. Agree on not backporting. Will you manually change the changelog section?

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Oh, never mind me, I realized that I used np 1.18.x for testing, so now trying the backport again.

And yes, I'll manually clean up changelog inconsistencies before a release is done.

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

(I still suspect that there are a bit too many absolute imports that may not work the way expected)

@bsipocz

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Now run into only one failure of comparing unicodes to strings, so still keep it left out from this batch of backports, but if needed this would not be too difficult to resolve:

___________________________________________________ test_rename_path ___________________________________________________

tmpdir = local('/private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/pytest-of-bsipocz/pytest-268/test_rename_path0')

    def test_rename_path(tmpdir):
    
        # Regression test for a bug that caused the path to the class to be
        # incorrect in a renamed model's __repr__.
    
        assert repr(RENAMED_MODEL).splitlines()[0] == "<class 'astropy.modeling.tests.test_core.CustomGaussian'>"
    
        # Make sure that when called from a user script, the class name includes
        # __main__.
    
        env = os.environ.copy()
        paths = [os.path.dirname(astropy.__path__[0])] + sys.path
        env['PYTHONPATH'] = os.pathsep.join(paths)
    
        script = tmpdir.join('rename.py').strpath
        with open(script, 'w') as f:
            f.write(MODEL_RENAME_CODE)
    
        output = subprocess.check_output([sys.executable, script], env=env)
>       assert output.splitlines() == MODEL_RENAME_EXPECTED.splitlines()
E       assert ["<class 'ast...ssian'>", ...] == ["<class 'astr...ssian'>", ...]
E         At index 2 diff: "Inputs: (u'x',)" != "Inputs: ('x',)"
E         Use -v to get the full diff

astropy/modeling/tests/test_core.py:426: AssertionError

bsipocz added a commit that referenced this pull request Jul 15, 2019

Merge pull request #8845 from astrofrog/find-current-module-bundle
Fix find_current_module so that it can work in application bundles
@pllim

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Looks like output.splitlines() is spewing out unicode in PY2, so maybe something like this?

list(map(str, output.splitlines()))

Disclaimer: Just a theoretical guess from a glance; untested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.