BUG: Ensure import astropy.coordinates works with -OO optimize flag

[pllim]

Description

This pull request is to ensure import astropy.coordinates does not crash when called with Python -OO flag because cls.__doc__ is None in that case. Just don't expect nice docstring for the affected API when you use that flag.

Fixes #15028

[github-actions]

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 "When to rebase and squash commits".
• 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?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• 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.

[pllim]

mpldev failure is unrelated

[taldcroft]

Thanks @pllim! From my perspective this is good and it accomplishes the minimal goal of making astropy functional when python is run with the -OO flag. It would be nice to see documentation in the PR description of the functional testing since there are no unit tests that cover this.

Whether this should be merged without some resolution of the other issues in #15028 is another question.

[pllim]

I thought about maybe adding a job to test for -OO imports but is that a good idea? Where? Maybe @astropy/astropy-project-release-team and @astropy/devops-maintainers have thoughts.

[eerovaher]

I'm not against merging this as is, but maybe it would be better to check sys.flags.optimize explicitly, in which case the new code comments would not be needed.

[pllim]

@eerovaher , are you saying to only run the affected code for sys.flags.optimize == 0 only?

[eerovaher]

Yes, except that the -O flag still handles docstring normally, so the actual condition to check would be sys.flags.optimize < 2.

[pllim]

But what does "Currently Nuitka sets optimize=1 for flag -OO" mean?

[mhvk]

This looks good, but should one of our tests be adjusted to run with -OO? Though I'm not sure pytest can work in that case!! Anyway, am approving here since I think it can be a separate issue.

[MatCat776]

But what does "Currently Nuitka sets optimize=1 for flag -OO" mean?

Nuitka is 3rd party software. Last week it incorrectly set optimize=1 when python flag is -OO. This problem was fixed in the factory release (not on pip yet) of Nuitka. I thought the problem I'm seeing in my code that uses both Nuitka and Astropy was caused by Astropy not handling the python -OO flag, but now I think there are 3 separate (but maybe related) issues: Astropy -OO problem (this PR), Nuitka optimize flag incorrect setting (fixed in Nuitka), and an unresolved issue.

[pllim]

an unresolved issue

Interesting, so those two patches together still did not fix everything? What else?

[MatCat776]

Interesting, so those two patches together still did not fix everything? What else?

I'm really not sure. My plan was to try to update ply and see if that helped, but I'm not getting anywhere with that due to my limited python skills. My next idea is to wait until this PR and Nuitka's fix are in the mainline versions of both code bases and try again with clean installs of each. (I don't have high hopes this will fix the problem because my local versions show improvement for simplified tests of each bug, so I think I applied the fixes correctly.)

[pllim]

Given the conversation thus far, I am not convinced that strict optimize flag check is any nicer than the existing one, as it seems like cls.__doc__ maybe be None for slight different scenario. Since there are already 2 approvals, let's get this in so we can smoke out the third problem, but @eerovaher, feel free to submit follow-up PR if you feel strongly about using optimize flag.

I will also open a follow up issue about testing. See #15052

Thank you, all!

[saimn]

io.fits (and other modules) already checks if __doc__ is None (or a string), since #3923. This seems the best option to me, compared to checking a specific value of sys.flags.optimize.

[eerovaher]

I suggested explicitly checking sys.flags because I thought that would make the code more self-documenting so that there would be no need to add comments, but neither of the updated Python files imports sys, and I can agree that importing it just to avoid writing two simple comments is excessive.