BUG: fix `generate_ref_ast.py` helper script and add installation instructions for it by neutrinoceros · Pull Request #16976 · astropy/astropy

neutrinoceros · 2024-09-09T14:02:34Z

Description

In #16932 we discovered that this script wasn't trivial to run, and was in fact broken. Here's a fix together with some installation instructions for future reference.
Independently, it looks like running the script now changes a couple data files, should I also commit those changes here @eerovaher ?

ref #16932 (comment)

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-09-09T14:03:03Z

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.

github-actions · 2024-09-09T14:03:06Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-09-09T14:07:08Z

astropy/coordinates/tests/accuracy/generate_ref_ast.py

-        " the reference values were computed using AST\n"
-    )
-    t.write(f, format="ascii", delimiter=",")
+    with open(DATA_DIR / fnout, "w") as fh:


the true bug was that the file was open in binary mode, when the following write functions both assume text mode. Everything else (using a context manager and an absolute path) is just for convenience.

Does this mean that the script that claims to have produced reference data for our tests cannot possibly have done that?

I'm not quite sure, it may have been able to run as is in Python 2, where strings and bytes objects behaved differently. Unfortunately I cannot test this (Python 2 doesn't compile on mac ARM).

I'm not familiar with Python 2 either. I suppose it's plausible enough that this was Python 2 script that has not been properly ported to Python 3.

neutrinoceros · 2024-09-09T16:24:59Z

Note that there's a PR upstream to overcome the current limitations with Python 3.13 and numpy 2 Starlink/starlink-pyast#45

eerovaher · 2024-09-09T18:49:41Z

Independently, it looks like running the script now changes a couple data files, should I also commit those changes here?

Very hard to say without seeing the changes.

Note that there's a PR upstream to overcome the current limitations with Python 3.13 and numpy 2 Starlink/starlink-pyast#45

It would be great if an upstream release could make the script work with a broader set of dependencies, but looking at the frequency of updates in starlink-pyast I don't think we should be waiting for them.

neutrinoceros · 2024-09-10T07:20:33Z

Very hard to say without seeing the changes.

@eerovaher Please see #16980

It would be great if an upstream release could make the script work with a broader set of dependencies, but looking at the frequency of updates in starlink-pyast I don't think we should be waiting for them.

agreed. I opened a reminder issue for this #16981

neutrinoceros · 2024-09-10T18:28:56Z

Thanks to @eerovaher's suggestions in #16980, this is now completely consistent: I regenerated the datafiles using the fixed script and committed the diff, which is only in the headers

eerovaher · 2024-09-10T18:42:51Z

astropy/coordinates/tests/accuracy/generate_ref_ast.py

+# /// script
+# requires-python = ">=3.10, <3.13"
+# dependencies = [
+#     "starlink-pyast==3.15.4",
+#     "numpy==1.26.4",
+#     "astropy",
+# ]
+# ///
+# above are PEP 723 script metadata for later reference, but they are
+# insufficient to run the script: starlink-pyast doesn't have wheels or proper
+# build time requirements specifications.
+# It must be build from source and without isolation, for instance, as follows
+# pip install 'numpy==1.26.4' 'setuptools==74.1.2'
+# pip install 'starlink-pyast==3.15.4' --no-build-isolation
+# pip install astropy


I'll try to find time tomorrow to follow these instructions.

I was able to successfully generate the data files by following these instructions with Python 3.10 (which installed astropy 6.1.3).

awesome. Anything else or is this good to go ?

astropy/coordinates/tests/accuracy/generate_ref_ast.py

…uctions for it

astropy/coordinates/tests/accuracy/generate_ref_ast.py

eerovaher

My understanding is that there were three problems with this script:

Apparently the script was originally written for Python 2, but was not ported to Python 3 properly.
It was not immediately obvious how the floating point numbers were formatted and apparently the default values used for formatting have changed.
Installing starlink-pyast is not trivial.

The first two problems are fixed here. The third one is an upstream problem, but this pull request adds adequate documentation to our script.

It might be a good idea to check if the other scripts we have for replicating test data work or not.

neutrinoceros · 2024-09-11T14:19:22Z

Nicely summarized, thanks !

It might be a good idea to check if the other scripts we have for replicating test data work or not.

Agreed, I can try and audit the codebase. If you have ideas of how to systematically discover such script, don't hesitate to share them !

eerovaher · 2024-09-11T14:27:14Z

One option would be to search for .py files in our tests/ directories whose name does not start with test_, but I have also seen instructions for replicating test data in docstrings of individual tests and I don't have a good idea how to find those.

neutrinoceros · 2024-09-11T14:29:00Z

ok, I guess it's still good if we can check some of them rather than none. Thanks !

neutrinoceros mentioned this pull request Sep 9, 2024

MNT: Run PTH flake test in prep for supporting pathlib (coordinates) #16932

Closed

1 task

github-actions bot added the coordinates label Sep 9, 2024

neutrinoceros force-pushed the coordinates/bug/fix_generate_ref_ast.py branch from 83ae082 to 39bd278 Compare September 9, 2024 14:05

neutrinoceros commented Sep 9, 2024

View reviewed changes

neutrinoceros changed the title ~~BUG: fix a generate_ref_ast.py helper script and add installation instructions for it~~ BUG: fix generate_ref_ast.py helper script and add installation instructions for it Sep 9, 2024

neutrinoceros force-pushed the coordinates/bug/fix_generate_ref_ast.py branch from 39bd278 to 1f4631b Compare September 9, 2024 14:07

neutrinoceros marked this pull request as ready for review September 9, 2024 14:12

eerovaher added testing no-changelog-entry-needed labels Sep 9, 2024

eerovaher added this to the v7.0.0 milestone Sep 9, 2024

This was referenced Sep 10, 2024

TST: update cooodinates accuracy test data #16980

Closed

Reminder: update installation instructions for generate_ref_ast.py #16981

Open

neutrinoceros force-pushed the coordinates/bug/fix_generate_ref_ast.py branch 2 times, most recently from 255b27e to bed6c46 Compare September 10, 2024 18:27

eerovaher reviewed Sep 10, 2024

View reviewed changes

neutrinoceros force-pushed the coordinates/bug/fix_generate_ref_ast.py branch from bed6c46 to 2c9d92c Compare September 11, 2024 09:01

BUG: fix generate_ref_ast.py helper script and add installation instr…

f1c58a1

…uctions for it

neutrinoceros force-pushed the coordinates/bug/fix_generate_ref_ast.py branch from 2c9d92c to f1c58a1 Compare September 11, 2024 11:02

eerovaher reviewed Sep 11, 2024

View reviewed changes

astropy/coordinates/tests/accuracy/generate_ref_ast.py Show resolved Hide resolved

eerovaher approved these changes Sep 11, 2024

View reviewed changes

eerovaher merged commit afe9a0c into astropy:main Sep 11, 2024

neutrinoceros deleted the coordinates/bug/fix_generate_ref_ast.py branch September 11, 2024 14:19

neutrinoceros mentioned this pull request Feb 25, 2026

MNT: update dependency metadata in generate_ref_ast.py #19328

Merged

1 task

Uh oh!

Conversation

neutrinoceros commented Sep 9, 2024

Description

Uh oh!

github-actions bot commented Sep 9, 2024

Uh oh!

github-actions bot commented Sep 9, 2024

Uh oh!

neutrinoceros Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Sep 9, 2024

Uh oh!

eerovaher commented Sep 9, 2024

Uh oh!

neutrinoceros commented Sep 10, 2024

Uh oh!

neutrinoceros commented Sep 10, 2024

Uh oh!

eerovaher Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Sep 11, 2024

Uh oh!

eerovaher commented Sep 11, 2024

Uh oh!

neutrinoceros commented Sep 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants