ENH: ensure that dir(astropy) lists subpackages by mhvk · Pull Request #17598 · astropy/astropy

mhvk · 2025-01-02T19:45:21Z

After import astropy, dir(astropy) will now list all subpackages, including those that have not yet been loaded explicitly. This also means tab completion will work as expected (e.g., from astropy.coo<TAB> will expand to from astropy.coordinates).

Fixes #17589

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-01-02T19:45:49Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

mhvk · 2025-01-02T19:46:29Z


-__dir_inc__ = [
-    "__version__",
-    "__githash__",


I had to remove this one since it is not actually defined and ipython seems to use __all__ when doing tab completion.

EDIT: indeed, this means that once you tab-complete, all subpackages are loaded. I think that's OK, since that's just interactive usage anyway.

Probably okay to remove. It was originally __dir__ back in #4020 but changed to __dir_inc__ in #7617 .

this means that once you tab-complete, all subpackages are loaded.

I think that's how the IPython REPL implements autocompletes for import statements, but I'd like to try it with Python 3.13's new REPL too.

Python 3.13's REPL (aka PyREPL) doesn't seem to even try autocompletion in this area, so no change there.
I agree that it's fine that IPython users will now implicitly load everything if they use , that is part of the IPython deal anyway.

I just check and I do get python 3.13.1 to auto-complete astropy.<TAB> - and it also autoloads them to show what is available. Indeed, it does just the logical thing: if I do astropy.sta<TAB> then it finds stats but astropy.time is not loaded.

Ah, indeed, confirmed that that does not work on python 3.13 (but does on ipython); I've noticed before that __getattr__/__dir__ don't quite work as nicely with from ... import ... - must involve a slightly different path or so.

python/cpython#129329 👀

python/cpython#129329 was just merged though I have no idea what Python versions it will land on. Anything we need to follow-up on still?

It'll be in 3.14. We can test this further then, but I don't anticipate any follow up to be needed.

Thanks for the info! I opened #18055 as reminder, just in case.

pllim · 2025-01-03T15:33:39Z

@celik-muhammed are you satisfied with this proposed solution?

pllim · 2025-01-03T16:00:11Z

Hmm seems to break RTD somehow.

<unknown>:15: WARNING: py:obj reference target not found: get_name [ref.obj]
<unknown>:15: WARNING: py:obj reference target not found: name [ref.obj]

pllim · 2025-01-03T16:23:34Z

pyinstaller is not happy, hmm, why?

______________________________ test_find_mod_objs ______________________________

    def test_find_mod_objs():
        deprecation_message = (
            "^The find_mod_objs function is deprecated and may be removed in "
            r"a future version\.$"
        )
        with pytest.warns(AstropyDeprecationWarning, match=deprecation_message):
>           lnms, fqns, objs = find_mod_objs("astropy")

astropy_tests/utils/tests/test_introspection.py:58: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
astropy/utils/decorators.py:141: in deprecated_func
    ???
astropy/utils/introspection.py:341: in find_mod_objs
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <list_iterator object at 0x7f02df7dfb50>

>   ???
E   KeyError: 'convolution'

astropy/utils/introspection.py:341: KeyError

pllim · 2025-01-03T16:24:53Z

Maybe this needs patching too?

astropy/astropy/utils/introspection.py

Line 341 in 80aa67d

pkgitems = [(k, mod.__dict__[k]) for k in mod.__all__]

mhvk · 2025-01-03T16:58:29Z

Nice catch on find_mod_objs - I pushed a fix... (clearly, there is a reason to have deprecated it -- it was written before module-level __getattr__...)

neutrinoceros

looks great. Just a couple minor suggestions from me !

neutrinoceros · 2025-01-03T17:00:24Z

+    module_dirs = {
+        f.name
+        for f in Path(astropy.__file__).parent.glob("[a-z]*")
+        if f.is_dir() and f.name != "extern"


Suggested change

if f.is_dir() and f.name != "extern"

if f.is_dir() and f.name != "extern" and f.joinpath("__init__.py").is_file()

I'd hope we won't add non-python module directories - if we ever do, then maybe that's the right time to add the extra clause... (similarly, in principle one can have modules defined in *.py files, which I also ignore; this just tests the existing scheme).

The reason I'm worried about this isn't that non-python module dirs are added to vcs, but that it might be done on a development copy and change this test's status possibly in a non-obvious way. I think it's worth spending a minute emaking the test narrow and correct now to save what would likely be a longer time spent on confused debugging for this hypothetical future contributor.

This isn't a blocking request though; if I haven't convinced you yet, then we're probably wasting more time discussing it that it'll ever save.

I see your point now, though perhaps not quite enough to run all CI again... Let's just get it in!

mhvk · 2025-01-03T17:56:58Z

Hmm seems to break RTD somehow.

<unknown>:15: WARNING: py:obj reference target not found: get_name [ref.obj]
<unknown>:15: WARNING: py:obj reference target not found: name [ref.obj]

Seems to be because of a bad changelog entry, from #17503. Not sure why that didn't get caught in testing before, though...

mhvk · 2025-01-03T17:59:49Z

I added two commits that should fix the RTD problems. Since these are not directly related to this PR (though somehow it exposes the problems!?), probably best not to squash-merge this PR.

mhvk · 2025-01-03T18:01:26Z

p.s. Because this fixes a previous changelog entry, the bot fails - so I put skip-changelog-checks - this part of course passed before, so that should be fine.

pllim · 2025-01-03T19:00:59Z

Hmm

  >       assert cp.returncode == 0
  E       AttributeError: 'int' object has no attribute 'returncode'

mhvk · 2025-01-03T19:27:15Z

Thanks! With that silly too-quick-to-adapt change fixed, tests seem to pass, including for readthedocs and pyinstaller (other OSes still running).

pllim

LGTM now! Unless @neutrinoceros has further comments, I think this is good to merge. I really like the idea of lazy import. Thanks!

celik-muhammed · 2025-01-03T20:37:56Z

sounds good, we keep lazy loading strategy then when we call dir(obj) it would loaded/listed submodules by getattr. It was a useful development.

neutrinoceros · 2025-02-12T18:57:06Z

+def test_toplevel_lazy_imports():
+    # Check that subpackages are loaded on demand.
+    cmd = dedent("""
+    import astropy, sys
+    assert 'astropy.units' not in sys.modules
+    astropy.units
+    assert 'astropy.units' in sys.modules
+    """)
+    cp = subprocess.check_call([sys.executable, "-c", cmd])
+    assert cp == 0


@mhvk @pllim looking for other sources of test pollution locally, I hit (I assume by chance) a failure mode for this test. It is extremely puzzling to me that this test might fail in a subprocess, let alone have anything to do with pollution, but unfortunately it is also very hard to debug; I don't even have a good traceback to work with. All I have is

subprocess.CalledProcessError: Command '['/Users/clm/dev/astropy-project/coordinated/astropy/.venv/bin/python3', '-c', "\nimport astropy, sys\nassert 'astropy.units' not in sys.modules\nastropy.units\nassert 'astropy.units' in sys.modules\n"]' returned non-zero exit status 1.

So, I'm pinging you guys in the hope that we can figure out a different test for this feature, but I for one don't have a clear idea how. I tried using monkeypatch.delitem(sys.modules, "astropy.units", raising=False), but it doesn't suffice to simulate "astropy.units has not been imported yet" and the second assert statement fails.

Actually, this should have its own issue. I'll open one now.

mhvk added the Enhancement label Jan 2, 2025

mhvk added this to the v7.1.0 milestone Jan 2, 2025

mhvk requested review from neutrinoceros and pllim January 2, 2025 19:45

github-actions bot added the testing label Jan 2, 2025

mhvk commented Jan 2, 2025

View reviewed changes

mhvk mentioned this pull request Jan 2, 2025

API: Allow dir(astropy) to list all subpackage names #17589

Closed

pllim reviewed Jan 3, 2025

View reviewed changes

Comment thread astropy/__init__.py

pllim added Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Jan 3, 2025

mhvk force-pushed the astropy-dir-getattr branch from a9ea40f to cf23549 Compare January 3, 2025 16:49

mhvk commented Jan 3, 2025

View reviewed changes

Comment thread astropy/__init__.py

neutrinoceros reviewed Jan 3, 2025

View reviewed changes

mhvk force-pushed the astropy-dir-getattr branch from 1d52ffc to b9dbbd7 Compare January 3, 2025 17:58

mhvk added the skip-changelog-checks Tells bot to skip changlog checks label Jan 3, 2025

mhvk added 5 commits January 3, 2025 14:13

ENH: ensure that dir(astropy) lists subpackages

06833cb

TST: ensure __all__ is complete

3de70a8

Ensure find_mod_objs can deal with module-level __getattr__

e84ca54

TST: update nitpick exceptions with new dict.update output

ebb7711

Fix changelog entry to avoid making references

8fd65e0

mhvk force-pushed the astropy-dir-getattr branch from b9dbbd7 to 8fd65e0 Compare January 3, 2025 19:13

pllim approved these changes Jan 3, 2025

View reviewed changes

mhvk merged commit 1531f4f into astropy:main Jan 5, 2025

mhvk deleted the astropy-dir-getattr branch January 5, 2025 14:45

neutrinoceros reviewed Feb 12, 2025

View reviewed changes

neutrinoceros mentioned this pull request Feb 12, 2025

TST: rare flakyness (or pollution) in a recently introduced subprocess-based test #17775

Open

pllim mentioned this pull request Apr 25, 2025

Revisit dir(astropy) implementation when Python 3.14 is minversion #18055

Open

	if f.is_dir() and f.name != "extern"
	if f.is_dir() and f.name != "extern" and f.joinpath("__init__.py").is_file()

Uh oh!

Conversation

mhvk commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 2, 2025

Uh oh!

mhvk Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 3, 2025

Uh oh!

Uh oh!

pllim commented Jan 3, 2025

Uh oh!

pllim commented Jan 3, 2025

Uh oh!

pllim commented Jan 3, 2025

Uh oh!

Uh oh!

mhvk commented Jan 3, 2025

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jan 3, 2025

Uh oh!

mhvk commented Jan 3, 2025

Uh oh!

mhvk commented Jan 3, 2025

Uh oh!

pllim commented Jan 3, 2025

Uh oh!

mhvk commented Jan 3, 2025

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

celik-muhammed commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

mhvk commented Jan 2, 2025 •

edited

Loading

mhvk Jan 2, 2025 •

edited

Loading