MNT: add `astropy.system_info` report helper function #16335

neutrinoceros · 2024-04-24T10:06:13Z

Description

This has been proposed a couple times in passing, so here's an implementation of this idea that's immediately useful (not just when every user can be assumed to have a sufficiently recent version of astropy that includes the new function).

this outputs a report looking like

platform
--------
platform.platform() = 'macOS-14.4.1-arm64-arm-64bit'
platform.version() = 'Darwin Kernel Version 23.4.0: Fri Mar 15 00:12:49 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6020'
platform.python_version() = '3.12.2'

packages
--------
astropy              7.0.0.dev41+gdcf60561fc
numpy                1.26.4
scipy                1.13.0
matplotlib           --
pandas               --
pyerfa               2.0.1.4

How to test this:

curl --silent https://raw.githubusercontent.com/neutrinoceros/astropy/mnt/report_info_script/astropy/utils/report_info.py | python

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 · 2024-04-24T10:06:41Z

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.

astrofrog · 2024-04-24T10:22:18Z

I like this idea in general, but I am wondering if there is any benefit to including the list of packages here versus just asking users to give the output of pip freeze?

astrofrog · 2024-04-24T10:25:36Z

This might also make more sense to put in https://github.com/astropy/astropy-tools - and it doesn't have to be core package specific, but could be used for coordinated packages too.

neutrinoceros · 2024-04-24T10:47:25Z

I am wondering if there is any benefit to including the list of packages here versus just asking users to give the output of pip freeze?

I can think of one: not everybody uses pip (conda and uv come to mind)

neutrinoceros · 2024-04-24T10:49:52Z

This might also make more sense to put in astropy/astropy-tools - and it doesn't have to be core package specific, but could be used for coordinated packages too.

I agree. However, I'm not sure how much additional complexity would be required to make it re-usable (the list of "relevant" packages would vary from one repo to the other)

neutrinoceros · 2024-04-24T12:23:32Z

Another reason why not to make this a shared resource is that I would like to add information about the build environment, eventually, but that's so project specific that I don't think such a section could be made portable, could it ?

pllim · 2024-04-24T13:40:59Z

curl isn't Windows friendly, so I don't think this would fly as it is even though it is a good idea.

neutrinoceros · 2024-04-24T13:45:06Z

Ah, thanks for pointing this out. I don't know off-hand how to change it but I definitely don't want to alienate such a common platform.

astrofrog · 2024-04-24T14:10:31Z

Thinking about this a bit more, this seems to overlap somewhat with pytest-astropy-header - certainly what we are printing here is very similar to what is there. Can we avoid duplication somehow? We could for instance ask users to install pytest and do:

pytest --pyargs astropy --astropy-header

which has output that includes:

Platform: Linux-6.5.0-28-generic-x86_64-with-glibc2.35

Executable: /home/tom/python/dev/bin/python3.11

Full Python Version: 
3.11.9 (main, Apr  6 2024, 17:59:24) [GCC 11.4.0]

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

Package versions: 
Numpy: 1.25.2
Scipy: 1.11.2
Matplotlib: 3.7.3
h5py: 3.10.0
Pandas: 2.1.0

anyway not saying that is the solution, but there certainly is a lot of overlap, and various packages have already put in the work to determine what is relevant to show in this list.

neutrinoceros · 2024-04-24T15:03:10Z

Ideally there should be as little friction as possible, and requiring pytest is a bit much in my opinion: users with broken envs or not able to install python packages (for example if they use a server where they don't have install rights) might turn away the minute we ask them to do something that doesn't work out of the box.

The invoke you're proposing also runs a large part of the test suite, which is a lot more than what my script intends to do.
Do you know of a way to ask pytest to output just the header ?

All that said, it might indeed be a good idea to avoid duplication, but I'm thinking it should be the other way around (pytest header would rely on this script), if possible.

pllim · 2024-04-24T15:11:28Z

I think there was a very old conversation about such thing but no one could agree on what is the one best way to pull it off. Every problem is a little different.

eerovaher · 2024-04-24T16:07:22Z

In our bug report template we ask the users to list the versions of some of the key packages, but the less code they need to run for that the better. Relying on any external code (whether it be from pytest-astropy-header or somewhere else) is not helpful for this purpose.

Having a single command that could be copy-pasted and executed in a shell would be good, but in practice it doesn't work on all platforms. Possible problems using curl on Windows were already mentioned, and independently from that Debian-based systems (e.g. Ubuntu) have python3 instead of python. It looks like the best we could do is to require users to execute two Python statements: the first to import a function and the second to run it.

neutrinoceros · 2024-04-24T16:52:10Z

It looks like the best we could do is to require users to execute two Python statements: the first to import a function and the second to run it.

That's indeed how I intend it to be used long term, when it is safe to assume that every user runs a sufficiently recent version of astropy that it has the function, but it can't be how we advertise it from the get go. What if w didn't try to use it in the bug report template for now (or maybe just suggest it in passing) ?

eerovaher · 2024-04-25T18:45:59Z

There's no LTS version anymore, so we no longer provide bugfixes for astropy versions older than 6 months. Furthermore, we recommend that users reporting a bug should verify that the bug is still present in the development version of astropy. That means that the transition period during which both new and old instructions should remain visible can be rather short.

neutrinoceros · 2024-04-26T07:47:25Z

Ok let's go with this plan then.

neutrinoceros · 2024-04-26T07:57:19Z

In order to avoid making it more confusing than it needs to, I streamlined the instructions into a try/except block. The script will only used if installed, so it can also serve as an indicator of where our users are at. And curl isn't needed any more, so it should behave the same on all platforms.

pllim · 2024-04-26T18:19:42Z

.github/ISSUE_TEMPLATE/bug_report.yaml

-            print("Matplotlib", matplotlib.__version__)
-        except ImportError:
-            print("Matplotlib not installed")
+            from astropy.utils.report_info import main; main()


Still not 100% sold on this. This import won't work until astropy 7.0 is released. Until then, we might just get confused users reporting that our bug report template is broken.

I agree with @astrofrog that https://github.com/astropy/astropy-tools is probably a better place for such scripts, but then the same problem about it not being readily installable.

This import won't work until astropy 7.0 is released.

except for reporters running the main branch, which I suspect wouldn't be a negligible fraction ?

Until then, we might just get confused users reporting that our bug report template is broken.

Now I'm confused: why would anyone report this as broken if there's no visible exception being raised ? (this isn't a rhetorical question, I'm genuinely curious)

Actually a lot of users report from stable release. Not everyone installs from main even when requested for various reasons (no time, no desire, stuck in old release, etc). Therefore for those users, from astropy.utils.report_info import main will not work until we release v7. Hope this clarifies the matter.

But this is in a try-except block.

Oh, oops... Must have missed it in the diff. Let me ponder again then when I get the chance. Thanks for your patience.

Would it be more explicit if we now do an astropy version check instead of a blind try-except?

More explicit yes, but maybe also more fragile, and I'm afraid it would come back biting us with users reporting the script is broken...

Well, we would need to test this script in CI anyway. And even then if it is still broken, won't we want to know? Under what condition would the script not break in CI but broken when user runs it? 🤔

Since the attribute astropy.__version__ is generated via setuptools_scm, I don't feel confortable taking any bets on exactly what could go wrong, but I don't trust that CI exercises all the ways it can be broken, which should all be weird edge cases that no one ever reported yet.

Additionally, I note that

the main function in the script has a doctest

the exact way it's used in the report template is not exercised yet (as far as I know). That's definitely doable if you think it's a good idea.

eerovaher · 2024-04-29T15:58:50Z

astropy/utils/report_info.py

+    pyerfa               ...
+    """
+    report_lines = chain(
+        header("platform"),


Why don't you call header() in the report_* functions?

separation of concerns, I guess, but I don't hold a strong opinion on this.

eerovaher · 2024-04-29T16:00:58Z

astropy/utils/report_info.py

+This can be run in two fashions either by downloading the latest version of this
+script and running it on the fly
+```
+curl --silent https://raw.githubusercontent.com/astropy/astropy/main/astropy/utils/report_info.py | python
+```


I'd remove this entirely. It is not possible to provide a simple shell one-liner that will work on all the supported platforms.

eerovaher · 2024-04-29T16:02:48Z

astropy/utils/report_info.py

+def header(name):
+    return [name, "-" * len(name)]


Adding type annotations is not mandatory, but I recommend adding them nonetheless:

Suggested change

def header(name):

return [name, "-" * len(name)]

def header(name: str) -> tuple[str, str]:

return name, len(name) * "-"

or perhaps

Suggested change

def header(name):

return [name, "-" * len(name)]

def header(name: str) -> str:

return f"{name}\n{len(name) * '-'}"

I'd like to keep the return type a list for two reasons:

in tuples, position is significant (borrowing the "Cultural Difference between lists and tuples" from https://nedbatchelder.com/blog/201608/lists_vs_tuples.html)

your second suggestion requires more changes

I'd prefer to keep the output immutable unless there is good reason to make it mutable, but these functions should never be called outside this module, so it doesn't really matter.

astropy/utils/report_info.py

eerovaher · 2024-04-29T16:09:18Z

astropy/utils/report_info.py

+    pandas               ...
+    pyerfa               ...
+    """
+    report_lines = chain(


I'm not convinced importing itertools just to use chain() here once is worth it.

I'm confused, what does it cost ? Last I checked itertools wasn't an expensive import, and it seems to be imported so often by other parts of the standard library that trying to avoid the overhead seemed futile.

chain() is not an overly expensive function, but in this case it is very simple to avoid importing itertools entirely:

Suggested change

report_lines = chain(

report_lines = "\n".join([*header("platform"), *report_platform(), "", *header("packages"), *report_packages()])

or if you move the header() calls inside the report_* functions it would be as simple as

Suggested change

report_lines = chain(

report_lines = "\n".join([*report_platform(), "", *report_packages()])

I've dropped itertools using unpacking. Thanks !

eerovaher · 2024-04-29T21:30:49Z

astropy/utils/report_info.py

+
+This can be run as
+```
+python -c "from astropy.utils.report_info import main; main()"


This might not work on Debian-based systems (including Ubuntu) because on those Python is by default installed as python3, not python.

So, what are you suggesting ? should I mention it in text or should I just present the 2-line python script as such ?

in passing, I'm tempted to go with python -m astropy.utils.report_info instead (already implemented)

eerovaher · 2024-04-30T21:27:17Z

I'd prefer if the function that prints the installed versions was named like either numpy.show_runtime() or possibly sunpy.system_info().

pllim · 2024-05-01T19:35:39Z

Thinking about this more, I am curious to see how other packages are rolling such things out. I have reached out to Scientific Python and will get back here after I have gathered some external data. Thank you for your continued patience!

pllim · 2024-05-03T16:09:15Z

My conclusion from various feedback is that the packages rolled their own reporting because each has different needs. So the script is probably okay in this repo, but maybe ditch the CLI and rename main() to something more descriptive (as Eero has mentioned above)?

scipy

scipy.show_config()

https://github.com/scipy/scipy/blob/1464ae12df417a8c8b6750d86fca4437d1c732c0/scipy/__config__.py.in#L122

https://github.com/scipy/scipy/blob/1464ae12df417a8c8b6750d86fca4437d1c732c0/scipy/__init__.py#L51

numpy

np.show_runtime()

https://github.com/numpy/numpy/blob/aa0cc043d1fe51e61024842b21905e11a2a4cee6/numpy/lib/_utils_impl.py#L20

Others

pyvista.Report()

mne.sys_info()

pllim · 2024-05-03T16:11:00Z

system_info like the one sunpy uses (ref #16335 (comment)) seems reasonable.

pllim · 2024-05-03T16:11:59Z

astropy/utils/report_info.py

Add an __all__ in this module as well?

neutrinoceros · 2024-05-03T17:35:32Z

maybe ditch the CLI

I'm not sure what you mean here. Are you refering to the curl invoke (already ditched but may still appear in outdated threads), or the if __name__ == "__main__": block ? (in the latter case, may I ask why ? I don't see the hurt in keeping it around)

Thanks for reminding me of @eerovaher's suggestion to change the name of the function, I think I missed it entirely !

neutrinoceros · 2024-05-03T17:45:35Z

I've renamed the module and its main function, added an __all__, and re-exposed the main function as astropy.system_info since having it at the package's top level seems to be the convention in the ecosystem.
The branch history should get squashed into a single commit with the same title as the PR. Happy to do this by hand if desired.

eerovaher · 2024-05-06T13:05:05Z

astropy/utils/system_info.py

+
+This can be run as
+```
+python -m astropy.utils.report_info


Suggested change

python -m astropy.utils.report_info

python -m astropy.utils.system_info

eerovaher · 2024-05-06T13:07:01Z

astropy/utils/system_info.py

+This script should not require anything else than the standard library.
+"""
+
+__all__ = ["system_info"]


Defining __all__ makes this module look public, so there should be an explicit warning in the module docstring that the contents here are subject to change without notice.

eerovaher · 2024-05-06T13:09:35Z

astropy/utils/system_info.py

+    """
+    Examples
+    --------
+    >>> main()  # doctest: +ELLIPSIS


Suggested change

>>> main() # doctest: +ELLIPSIS

>>> import astropy

>>> astropy.system_info()

I have a vague memory that we don't need +ELLIPSIS because it is turned on globally somewhere in our configuration, but do check if this is true.

eerovaher · 2024-05-06T13:13:46Z

astropy/utils/system_info.py

+from importlib.util import find_spec
+
+
+def header(name: str) -> list[str]:


I'd prefer if all the functions except system_info() would be explicitly marked as private:

Suggested change

def header(name: str) -> list[str]:

def _header(name: str) -> list[str]:

astropy/utils/system_info.py

pllim · 2024-05-06T18:26:44Z

astropy/__init__.py

@@ -234,3 +234,6 @@ def online_help(query):
        del locals()[varname]

 del varname, __module_type__
+
+
+from astropy.utils.system_info import system_info


Does this also need to be updated?

astropy/astropy/__init__.py

Line 199 in e7a556f

__dir_inc__ = [

you tell me. I have never seen this variable name before, I don't know what it does.

Looks like @saimn renamed it from __dir__ in #7617 . Do you remember why we need this, Simon?

Yes, module.__dir__ has a special meaning since Python 3.7: https://docs.python.org/3.12/reference/datamodel.html#customizing-module-attribute-access

Oh, I see. I think my new function does belong in __dir_inc__, which lists everything that should not be dropped from the namespace, but since I added it at the end of the module, it's only included after namespace cleanup happens, so it dodges that. I think the cleanup ought to be the last thing to happen in the module, so I'll move my import and update this list, thanks !

pllim · 2024-05-06T18:27:46Z

astropy/utils/system_info.py

+    return 0
+
+
+if __name__ == "__main__":


I still don't see why we need this part.

Enabling running the module as a script (python -m astropy.utils.system_info) makes basically everything in the module (other than its name) an implementation detail, which seemed fitting.

@eerovaher , what do you think?

I wouldn't have implemented this. The ability to run

>>> import astropy >>> astropy.system_info()

in Python should be good enough.

Fair enough I'll drop it

pllim · 2024-05-06T18:28:44Z

in the latter case, may I ask why ? I don't see the hurt in keeping it around

Just one more thing we have to maintain but not advertised to be used in the template anyway.

neutrinoceros · 2024-05-06T18:50:39Z

Well, following @eerovaher's suggestion I added a note that the module is subject to change without notice, and it's just one line, so it can be thrown away the minute it causes any hurdle. That said, it's also not a hill I'll die on so if you're still unconvinced, I'll drop it.

eerovaher · 2024-05-07T14:32:31Z

astropy/utils/system_info.py

+    return lines
+
+
+def system_info() -> int:


There is no need to return 0 anymore.

Suggested change

def system_info() -> int:

def system_info() -> None:

pllim

Turns out its API doc is currently not rendered, so I think you have to add this module under "utility" in https://github.com/astropy/astropy/blob/main/docs/utils/ref_api.rst .

@stefanv over at Scientific Python suggested we could also parse pyproject.toml to gather the list of dependencies. Maybe something we can consider in the future if hardcoding is not enough.

Thanks for your patience with all the comments!

astropy/utils/system_info.py

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

neutrinoceros · 2024-05-08T07:05:23Z

I rebased and squashed all but the last two (new) commits, which correspond to applying latest suggestions from @pllim

pllim

Looks like pre-commit isn't happy about something.

I think this counts as a new feature under utils, so need a change log.

Not sure why the files diff has codecov complains. When I go into codecov page, says patch coverage is 100%. 🤷‍♀️

astropy/utils/system_info.py

pllim

LGTM. Thanks!

github-actions bot added utils dev-automation labels Apr 24, 2024

neutrinoceros force-pushed the mnt/report_info_script branch from ce8e889 to 42c6f24 Compare April 24, 2024 12:30

pllim added this to the v7.0.0 milestone Apr 24, 2024

neutrinoceros force-pushed the mnt/report_info_script branch from 42c6f24 to 1805b68 Compare April 26, 2024 07:54

pllim reviewed Apr 26, 2024

View reviewed changes

neutrinoceros force-pushed the mnt/report_info_script branch from 1805b68 to 8326880 Compare April 27, 2024 15:50

eerovaher reviewed Apr 29, 2024

View reviewed changes

pllim reviewed May 3, 2024

View reviewed changes

astropy/utils/report_info.py Outdated

Copy link

Member

pllim May 3, 2024

Choose a reason for hiding this comment

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

Add an __all__ in this module as well?

neutrinoceros reacted with thumbs up emoji

neutrinoceros changed the title ~~MNT: add astropy/utils/report_info.py helper script~~ MNT: add astropy.system_info report helper function May 3, 2024

eerovaher reviewed May 6, 2024

View reviewed changes

astropy/utils/system_info.py Outdated Show resolved Hide resolved

pllim reviewed May 6, 2024

View reviewed changes

eerovaher reviewed May 7, 2024

View reviewed changes

pllim reviewed May 7, 2024

View reviewed changes

astropy/utils/system_info.py Outdated Show resolved Hide resolved

astropy/utils/system_info.py Outdated Show resolved Hide resolved

astropy/utils/system_info.py Outdated Show resolved Hide resolved

neutrinoceros and others added 3 commits May 8, 2024 09:04

MNT: add astropy.system_info report helper function

70fc63b

Apply suggestions from code review

01806ad

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

list new module in api ref docs

550b3e9

neutrinoceros force-pushed the mnt/report_info_script branch from 5d2c635 to 550b3e9 Compare May 8, 2024 07:05

pllim reviewed May 8, 2024

View reviewed changes

astropy/utils/system_info.py Outdated Show resolved Hide resolved

neutrinoceros and others added 4 commits May 8, 2024 17:52

fixup! Apply suggestions from code review

c859d7b

fix docstring

7f8322e

add changelog fragment

4c9977f

DOC: Add blank line directive for doctest

c200bb3

pllim approved these changes May 8, 2024

View reviewed changes

pllim merged commit a285546 into astropy:main May 8, 2024
26 of 27 checks passed

pllim mentioned this pull request May 8, 2024

bug_report issue template: Use system_info exclusively when we drop support for astropy v6.x #16418

Open

neutrinoceros deleted the mnt/report_info_script branch May 9, 2024 06:11

	report_lines = chain(
	report_lines = "\n".join([header("platform"), report_platform(), "", header("packages"), report_packages()])

	report_lines = chain(
	report_lines = "\n".join([report_platform(), "", report_packages()])

	python -m astropy.utils.report_info
	python -m astropy.utils.system_info

	>>> main() # doctest: +ELLIPSIS
	>>> import astropy
	>>> astropy.system_info()

		from importlib.util import find_spec


		def header(name: str) -> list[str]:

	def header(name: str) -> list[str]:
	def _header(name: str) -> list[str]:

MNT: add astropy.system_info report helper function #16335

MNT: add astropy.system_info report helper function #16335

Conversation

neutrinoceros commented Apr 24, 2024

Description

github-actions bot commented Apr 24, 2024

astrofrog commented Apr 24, 2024

astrofrog commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024

pllim commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024

astrofrog commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024 • edited

pllim commented Apr 24, 2024

eerovaher commented Apr 24, 2024

neutrinoceros commented Apr 24, 2024

eerovaher commented Apr 25, 2024

neutrinoceros commented Apr 26, 2024

neutrinoceros commented Apr 26, 2024

Choose a reason for hiding this comment

neutrinoceros Apr 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerovaher commented Apr 30, 2024

pllim commented May 1, 2024

pllim commented May 3, 2024 • edited

scipy

numpy

Others

pllim commented May 3, 2024 • edited

Choose a reason for hiding this comment

neutrinoceros commented May 3, 2024

neutrinoceros commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented May 6, 2024

neutrinoceros commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

neutrinoceros commented May 8, 2024 • edited

pllim left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

MNT: add `astropy.system_info` report helper function #16335

MNT: add `astropy.system_info` report helper function #16335

neutrinoceros commented Apr 24, 2024 •

edited

neutrinoceros Apr 27, 2024 •

edited

pllim commented May 3, 2024 •

edited

pllim commented May 3, 2024 •

edited

neutrinoceros commented May 8, 2024 •

edited