MNT: Make asdf import truly optional in io.misc.asdf by pllim · Pull Request #10260 · astropy/astropy

pllim · 2020-05-01T22:46:24Z

Description

This pull request is to address asdf import errors reported in #10246. Also fixed PEP 8 in the files I touched.

__________________________________ ERROR collecting astropy/io/misc/asdf/extension.py __________________________________
astropy/io/misc/asdf/extension.py:6: in <module>
    from asdf.extension import AsdfExtension, BuiltinExtension
E   ModuleNotFoundError: No module named 'asdf'
____________________________________ ERROR collecting astropy/io/misc/asdf/types.py ____________________________________
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
___________________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/angle.py ____________________________
astropy/io/misc/asdf/tags/coordinates/angle.py:6: in <module>
    from astropy.io.misc.asdf.tags.unit.quantity import QuantityType
astropy/io/misc/asdf/tags/unit/quantity.py:7: in <module>
    from asdf.tags.core import NDArrayType
E   ModuleNotFoundError: No module named 'asdf'
_______________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/earthlocation.py ________________________
astropy/io/misc/asdf/tags/coordinates/earthlocation.py:5: in <module>
    from ...types import AstropyType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
___________________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/frames.py ___________________________
astropy/io/misc/asdf/tags/coordinates/frames.py:6: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
_______________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/representation.py _______________________
astropy/io/misc/asdf/tags/coordinates/representation.py:5: in <module>
    from astropy.io.misc.asdf.types import AstropyType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
__________________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/skycoord.py __________________________
astropy/io/misc/asdf/tags/coordinates/skycoord.py:6: in <module>
    from ...types import AstropyType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
_______________________________ ERROR collecting astropy/io/misc/asdf/tags/fits/fits.py ________________________________
astropy/io/misc/asdf/tags/fits/fits.py:9: in <module>
    from astropy.io.misc.asdf.types import AstropyType, AstropyAsdfType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
______________________________ ERROR collecting astropy/io/misc/asdf/tags/table/table.py _______________________________
astropy/io/misc/asdf/tags/table/table.py:5: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
_______________________________ ERROR collecting astropy/io/misc/asdf/tags/time/time.py ________________________________
astropy/io/misc/asdf/tags/time/time.py:7: in <module>
    from asdf.versioning import AsdfSpec
E   ModuleNotFoundError: No module named 'asdf'
_____________________________ ERROR collecting astropy/io/misc/asdf/tags/time/timedelta.py _____________________________
astropy/io/misc/asdf/tags/time/timedelta.py:9: in <module>
    from ...types import AstropyType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
____________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/basic.py _____________________________
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
___________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/compound.py ___________________________
astropy/io/misc/asdf/tags/transform/compound.py:3: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
______________________ ERROR collecting astropy/io/misc/asdf/tags/transform/functional_models.py _______________________
astropy/io/misc/asdf/tags/transform/functional_models.py:7: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
_____________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/math.py _____________________________
astropy/io/misc/asdf/tags/transform/math.py:10: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
_______________________ ERROR collecting astropy/io/misc/asdf/tags/transform/physical_models.py ________________________
astropy/io/misc/asdf/tags/transform/physical_models.py:8: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
__________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/polynomial.py __________________________
astropy/io/misc/asdf/tags/transform/polynomial.py:7: in <module>
    from asdf.versioning import AsdfVersion
E   ModuleNotFoundError: No module named 'asdf'
__________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/powerlaws.py ___________________________
astropy/io/misc/asdf/tags/transform/powerlaws.py:7: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
_________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/projections.py __________________________
astropy/io/misc/asdf/tags/transform/projections.py:7: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
___________________________ ERROR collecting astropy/io/misc/asdf/tags/transform/tabular.py ____________________________
astropy/io/misc/asdf/tags/transform/tabular.py:9: in <module>
    from .basic import TransformType
astropy/io/misc/asdf/tags/transform/basic.py:4: in <module>
    from asdf import tagged
E   ModuleNotFoundError: No module named 'asdf'
____________________________ ERROR collecting astropy/io/misc/asdf/tags/unit/equivalency.py ____________________________
astropy/io/misc/asdf/tags/unit/equivalency.py:8: in <module>
    from astropy.io.misc.asdf.types import AstropyType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'
_____________________________ ERROR collecting astropy/io/misc/asdf/tags/unit/quantity.py ______________________________
astropy/io/misc/asdf/tags/unit/quantity.py:7: in <module>
    from asdf.tags.core import NDArrayType
E   ModuleNotFoundError: No module named 'asdf'
_______________________________ ERROR collecting astropy/io/misc/asdf/tags/unit/unit.py ________________________________
astropy/io/misc/asdf/tags/unit/unit.py:7: in <module>
    from astropy.io.misc.asdf.types import AstropyAsdfType
astropy/io/misc/asdf/types.py:4: in <module>
    from asdf.types import CustomType, ExtensionTypeMeta
E   ModuleNotFoundError: No module named 'asdf'

With this patch, all io.misc.asdf tests are skipped without collection error when asdf is not installed. But I am not familiar with how asdf works, so review from experts would be greatly appreciated.

cc @eslavich

TODO

If this is acceptable, does this need a change log after milestone is set?

eslavich · 2020-05-01T22:53:38Z

@pllim this also works, if we add it to a conftest.py file in astropy/io/misc/asdf:

from pathlib import Path
try:
    import asdf
except ImportError:
    # If asdf isn't available, instruct pytest to ignore
    # files in this module that may contain unguarded
    # imports.
    collect_ignore = [str(p) for p in Path(__file__).parent.glob("**/*.py")]

It has the advantage of maintaining itself, and keeping some clutter out of the other .py files in that subdirectory. On the other hand, it only fixes pytest collection, not other cases where an attempt to import the module fails.

pllim · 2020-05-01T23:00:02Z

My personal opinion: Astropy's only hard dependency at run-time is Numpy, therefore it seems ungraceful to have import error when accessing astropy.io.misc.asdf, but that said, I have no strong opinion either way, so I'll defer to @bsipocz .

bsipocz · 2020-05-01T23:24:38Z

With the conftest solution I still get the same collection errors, at least when using the latest pytest, etc.

bsipocz · 2020-05-01T23:30:48Z

I would very much like to move ahead with #7939, which would mean asdf shouldn't be a more special case then the rest of our optional dependencies. One can argue that the functionality in the subpackage is not there when it's not installed, but that's the same with the hdf5 submodule which doesn't produce this issue.

bsipocz · 2020-05-01T23:32:02Z

As of milestone, I think 4.1 is appropriate as this is a cleanup without functionality change.

pllim · 2020-05-02T00:55:40Z

astropy/io/misc/asdf/tags/coordinates/spectralcoord.py

-            node['value'] = custom_tree_to_tagged_tree(spec_coord.value, ctx)
-            node['unit'] = custom_tree_to_tagged_tree(spec_coord.unit, ctx)
-            node['observer'] = custom_tree_to_tagged_tree(spec_coord.observer, ctx)
-            node['target'] = custom_tree_to_tagged_tree(spec_coord.target, ctx)


Is this correct, @eslavich ?

cc @astrofrog

bsipocz · 2020-05-04T08:03:14Z

Interestingly there are 4 remaining ones when testing with your doctestplus PR+pytest5.4. I'm not 100% certain that it's relevant to this PR, as with that plugin PR the test collection should not fail with import issues.
Anyway, I'll investigate tomorrow.

$ ~/munka/devel/astropy [:bed0e8dd1|⚑ 23] $ pytest
================================================= test session starts ==================================================
platform darwin -- Python 3.8.0, pytest-5.3.5, py-1.8.0, pluggy-0.13.1
Matplotlib: 3.2.1
Freetype: 2.6.1

Running tests with Astropy version 4.1.dev1574+gbed0e8dd1.
Running tests in astropy docs.

Date: 2020-05-04T00:58:56

Platform: macOS-10.13.6-x86_64-i386-64bit

Executable: /Users/bsipocz/.pyenv/versions/3.8.0/bin/python3.8

Full Python Version: 
3.8.0 (default, Dec 26 2019, 15:55:54) 
[Clang 10.0.0 (clang-1000.11.45.5)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 1.18.2
Scipy: 1.4.1
Matplotlib: 3.2.1
h5py: 2.10.0
Pandas: 0.25.3

Using Astropy options: remote_data: none.

rootdir: /Users/bsipocz/munka/devel/astropy, inifile: setup.cfg, testpaths: astropy, docs
plugins: mpl-0.11, arraydiff-0.3, remotedata-0.3.2, filter-subpackage-0.1.1, openfiles-0.5.0, hypothesis-5.5.1, astropy-header-0.1.2, cov-2.8.1, doctestplus-0.7.0.dev0
collected 14984 items / 4 errors / 30 skipped / 14950 selected                                                         

======================================================== ERRORS ========================================================
__________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/tests/test_earthlocation.py __________________
ImportError while importing test module '/Users/bsipocz/munka/devel/astropy/astropy/io/misc/asdf/tags/coordinates/tests/test_earthlocation.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
astropy/io/misc/asdf/tags/coordinates/tests/test_earthlocation.py:7: in <module>
    from asdf.tests.helpers import assert_roundtrip_tree
E   ModuleNotFoundError: No module named 'asdf.tests.helpers'
____________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/tests/test_skycoord.py _____________________
ImportError while importing test module '/Users/bsipocz/munka/devel/astropy/astropy/io/misc/asdf/tags/coordinates/tests/test_skycoord.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
astropy/io/misc/asdf/tags/coordinates/tests/test_skycoord.py:12: in <module>
    from asdf.tests.helpers import assert_roundtrip_tree
E   ModuleNotFoundError: No module named 'asdf.tests.helpers'
__________________ ERROR collecting astropy/io/misc/asdf/tags/coordinates/tests/test_spectralcoord.py __________________
ImportError while importing test module '/Users/bsipocz/munka/devel/astropy/astropy/io/misc/asdf/tags/coordinates/tests/test_spectralcoord.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
astropy/io/misc/asdf/tags/coordinates/tests/test_spectralcoord.py:11: in <module>
    from asdf.tests.helpers import assert_roundtrip_tree  # noqa
E   ModuleNotFoundError: No module named 'asdf.tests.helpers'
_______________________ ERROR collecting astropy/io/misc/asdf/tags/time/tests/test_timedelta.py ________________________
ImportError while importing test module '/Users/bsipocz/munka/devel/astropy/astropy/io/misc/asdf/tags/time/tests/test_timedelta.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
astropy/io/misc/asdf/tags/time/tests/test_timedelta.py:7: in <module>
    from asdf.tests.helpers import assert_roundtrip_tree
E   ModuleNotFoundError: No module named 'asdf.tests.helpers'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 4 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================ 30 skipped, 4 errors in 16.38s ============================================

pllim · 2020-05-04T16:00:22Z

@bsipocz , at least one of the test files have asdf = pytest.importorskip('asdf') on top, so I thought pytest would skip them before trying to import asdf. 🤷

eslavich · 2020-05-04T16:35:56Z

@bsipocz I tried this PR on:

platform darwin -- Python 3.8.0, pytest-5.4.1, py-1.8.1, pluggy-0.13.1

and also

platform darwin -- Python 3.8.0, pytest-5.3.5, py-1.8.1, pluggy-0.13.1

each with doctestplus master and minimum dependencies and was able to collect everything without errors.

bsipocz · 2020-05-04T20:35:41Z

@eslavich - interesting. I suspect something is messed up for me locally, as #10237 seems to be working, too.

pllim · 2020-05-05T02:54:41Z

@bsipocz , I am fine if you want to remilestone this one. No rush on my side. FYI.

bsipocz · 2020-05-05T03:48:35Z

@pllim - I feel this can be a backport, but it's fine to change to the final milestone when we get to pushing the green button.

jdavies-st · 2020-05-05T14:58:06Z

So is the purpose of this PR to not get pytest collection errors? I believe there's much more simple ways to do this. I.e. --doctest-ignore-import-errors or explicit avoidance of modules on a case-by-case basis via setup.cfg. Those are standard to pytest and don't require much (or any) roll-your-own code. I agree with #10246 (comment)

In the end, this PR ends up raising ImportError instead of the original ModuleNotFoundError with probably a slightly more useful error message if one tries to do the imports directly without having asdf installed. But this is also a lot of code complexity to add in order to achieve this in my view. I think what happens currently on master:

In [1]: from astropy.io.misc.asdf import extension                                                                                    
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-2-fac6e72e8a75> in <module>
----> 1 from astropy.io.misc.asdf import extension

~/dev/astropy/astropy/io/misc/asdf/extension.py in <module>
      4 import os
      5 
----> 6 from asdf.extension import AsdfExtension, BuiltinExtension
      7 from asdf.resolver import Resolver, DEFAULT_URL_MAPPING
      8 from asdf.util import filepath_to_url

ModuleNotFoundError: No module named 'asdf'

is probably pretty clear for most debugging purposes.

pllim · 2020-05-05T15:02:09Z

My personal opinion is a more philosophical one, so I'll defer to @bsipocz since she originally reported the problem.

eslavich

I have a couple of suggestions for changes, but nothing terribly important.

I don't yet understand the need to import these modules, since even when asdf is installed the code is automatically loaded via entry points. Users shouldn't need to access any of this directly. If there doesn't turn out to be a use case for importing, then I'd prefer to solve the problem with a pytest feature like @jdavies-st suggests.

eslavich · 2020-05-05T16:23:20Z

astropy/io/misc/asdf/extension.py

+    HAS_ASDF = False
+    AsdfExtension = object
+    BuiltinExtension = object
+    ASTROPY_SCHEMA_URI_BASE = ''


ASTROPY_SCHEMA_URI_BASE and SCHEMA_PATH don't use anything from the asdf package, so they can continue to live outside of the try/except/else.

eslavich · 2020-05-05T16:25:23Z

astropy/io/misc/asdf/extension.py

+else:
+    HAS_ASDF = True

+    # Make sure that all tag implementations are imported by the time we create


I think we can continue to import these even if asdf is missing, since each of the individual modules already needs to be importable so that pytest can collect from them. I tested moving them back outside the try/except/else and pytest doesn't complain.

eslavich · 2020-05-05T16:28:53Z

astropy/io/misc/asdf/extension.py

+    ASTROPY_SCHEMA_URI_BASE = 'http://astropy.org/schemas/'
+    SCHEMA_PATH = os.path.abspath(
+        os.path.join(os.path.dirname(__file__), 'data', 'schemas'))
+    ASTROPY_URL_MAPPING = [


Another option here is to just compute this inside of AsdfExtension.url_mapping below. When asdf loads an extension, it only calls that method once, so there's no performance concern.

eslavich · 2020-05-05T16:31:50Z

astropy/io/misc/asdf/tags/time/time.py

    assert_array_equal(a.lon, b.lon)


+# Cannot use super() in __init__ to check for asdf import here because it


We wouldn't need to anyway since AstropyAsdfType already provides an __init__ that checks for asdf?

pllim · 2020-05-05T17:55:15Z

Thanks! I'll wait for a resolution whether we should move forward with this or not before I spend time on another commit.

nden · 2020-05-06T13:38:49Z

In order to write an object to ASDF or to read an ASDF file, the first command a user needs is
import asdf. I'm just repeating what others said above but yes, the code in io.misc.asdf is not usable on its own without asdf, i.e. there's no reason for a user to execute a command like
from astropy.io.misc import asdf. This PR does not help a user in any way and the only reason for this PR is to solve the problem with the test collection. OTH the PR adds a lot of unnecessary code. I strongly agree with the approach @jdavies-st and @eslavich suggested, i.e. pytest-doctestplus should ignore these modules. There shouldn't be any doctests in the modules anyway, they are not meant to be user visible.

pllim requested review from bsipocz, cshanahan1, jdavies-st and nden May 1, 2020 22:46

astropy-bot bot added io.misc zzz 💤 io.misc.asdf archived: Go to asdf-astropy repo labels May 1, 2020

pllim mentioned this pull request May 1, 2020

Tests should collect without optional dependencies #10246

Open

bsipocz added the no-changelog-entry-needed label May 1, 2020

bsipocz added this to the v4.1 milestone May 1, 2020

This comment has been minimized.

Sign in to view

pllim force-pushed the optional-import-asdf branch from 2e482b7 to b4e2960 Compare May 2, 2020 00:40

This comment has been minimized.

Sign in to view

pllim force-pushed the optional-import-asdf branch from b4e2960 to 21c5048 Compare May 2, 2020 00:53

pllim commented May 2, 2020

View reviewed changes

eslavich mentioned this pull request May 4, 2020

Handle older versions of transform/constant schema #10293

Merged

pllim mentioned this pull request May 4, 2020

No error if import fails unconditionally scientific-python/pytest-doctestplus#108

Merged

pllim added 2 commits May 4, 2020 11:54

Make asdf import truly optional in io.misc.asdf

654ac63

Follow up of astropy#10185 and apply astropy#10189 to it

d7cc3b4

pllim force-pushed the optional-import-asdf branch from 21c5048 to d7cc3b4 Compare May 4, 2020 15:54

pllim mentioned this pull request May 5, 2020

Speed up wcsaxes import by avoiding unnecessary pytest import #10302

Closed

eslavich reviewed May 5, 2020

View reviewed changes

pllim closed this May 7, 2020

pllim deleted the optional-import-asdf branch May 7, 2020 18:39

This was referenced May 7, 2020

MNT: PEP 8 clean-ups of some io.misc.asdf #10324

Merged

TST: Globally ignore import error during test collection for doctest #10325

Closed

Of required and optional dependencies #10342

Open

		assert_array_equal(a.lon, b.lon)


		# Cannot use super() in __init__ to check for asdf import here because it

Uh oh!

Conversation

pllim commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

eslavich commented May 1, 2020

Uh oh!

pllim commented May 1, 2020

Uh oh!

bsipocz commented May 1, 2020

Uh oh!

bsipocz commented May 1, 2020

Uh oh!

bsipocz commented May 1, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

pllim May 2, 2020

Choose a reason for hiding this comment

Uh oh!

pllim May 2, 2020

Choose a reason for hiding this comment

Uh oh!

eslavich May 2, 2020

Choose a reason for hiding this comment

Uh oh!

bsipocz commented May 4, 2020

Uh oh!

pllim commented May 4, 2020

Uh oh!

eslavich commented May 4, 2020

Uh oh!

bsipocz commented May 4, 2020

Uh oh!

pllim commented May 5, 2020

Uh oh!

bsipocz commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdavies-st commented May 5, 2020

Uh oh!

pllim commented May 5, 2020

Uh oh!

eslavich left a comment

Choose a reason for hiding this comment

Uh oh!

eslavich May 5, 2020

Choose a reason for hiding this comment

Uh oh!

eslavich May 5, 2020

Choose a reason for hiding this comment

Uh oh!

eslavich May 5, 2020

Choose a reason for hiding this comment

Uh oh!

eslavich May 5, 2020

Choose a reason for hiding this comment

Uh oh!

pllim commented May 5, 2020

Uh oh!

nden commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pllim commented May 1, 2020 •

edited

Loading

bsipocz commented May 5, 2020 •

edited

Loading