Remove js code bundled with Astropy for DataTables - #17521

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.

This is an alternative, slightly more radical version of #17480: It removes the bundled js code immediately instead of updating; keeping the astropy repository small.

@olebole If you could have a look if this has an impact on how you put astropy on Debian, that would be great.

p.s. Readthedocs failure seems real, but it not very informative:

<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]

I think the changelog entry should be api not other. The API changelog entry should indicate the change of the default use_local_files from True to False. Along with that include a brief description of what that implies for the user, that they are getting a newer version of DataTables and it always comes from an online CDN server. Finally that use_local_files=True will generate a warning and then get the online version anyway. Also mention this change is driven by a security issue.

With a detailed description of this change in the changelog, then IMO we are OK with a security-driven API change in a bugfix release.

Locally, I get the following sphinx failures:

/Users/guenther/code/astropy/astropy/config/paths.py:docstring of astropy.config.paths.get_cache_dir_path:1: WARNING: py:class reference target not found: pathlib._local.Path [ref.class]
/Users/guenther/code/astropy/astropy/config/paths.py:docstring of astropy.config.paths.get_config_dir_path:1: WARNING: py:class reference target not found: pathlib._local.Path [ref.class]

slightly more informative, but clearly nothing to do with this PR. I'm hope that it's just intersphinx that is flaky and I'm force-pushing new version which will trigger the tests again anyway. Maybe that's enough to pass.

Please rebase again to pick up new fixes to CI. I apologize for any inconvenience caused! (p.s. At least RTD was passing again)

I don't think we can backport this. I don't understand why it is milestoned to v7.0.x

I don't think we can backport this. I don't understand why it is milestoned to v7.0.x

Is there a technical problem or are you concerned about backporting an API change?

The latter.

This was discussed offline. Now I am okay with backporting this if downstream maintainers are okay.

I really dislike that most recently astropy keeps breaking backport and deprecation policies. Besides the removal of the bundled files it adds deprecations to the python code. If there is no technical reason that makes this a must and immediate removal then please keep it milestoned as expected, into 7.1 as to minimize out-of-schedule precedents of breaking the policies to the absolutely necessary level.

I don't think we should backport such a change to 7.0.1. This is a breaking change so goes against our release policy. Also this feature has been in astropy for a long time (#1342, >10 years ago), and #10420 has been opened for 4 years so I think we can wait a bit more.

I don't think we should backport such a change to 7.0.1. This is a breaking change so goes against our release policy.

show_in_browser() is already completely broken, so one can reasonably argue that it is not possible for this release to break something that already does not work.

>>> astropy.__version__
7.0.0
>>> t.show_in_browser(jsviewer=True)
...
TypeError: quote_from_bytes() expected bytes

it's broken in the new release, it works for me for a previous version and doesn't issue a deprecation warning. So having a new bug is not a legitimate reason for complete removal, in case someone wants to reuse this case as a precedent for future removals.

show_in_browser() is already completely broken

because pathlib updates introduced a few bugs, this one is already fixed on the release branch.

Yes. For posterity this is #17513

@pllim This is a classic lock where we don't agree which PR to take, though I believe that all interested parties have given their opinion, some here and some in private discussion. APE0 says that one of the coco's roles is to make decisions in case no agreement is reached. Since you, @pllim , have been part of this discussion, and, as far as I know, most of the side-conversation in other channels that are not (yet) fully documented in this PR, @taldcroft and I want to suggest that you take this to the Coco to make a decision between this PR and #17480.

(If the Coco picks #17480, that probably needs a little bit more work in terms of docs and changelog, but I can do that pretty quickly - I just don't want to polish both PRs perfectly, if we take only one of them. In that case, I would rebase this PR after #17480 is merged and we would take this PR for a later release following the normal deprecation cycle.)

While CoCo ponder this, here is the deal: No one is against removing JS for v7.1, so nothing stopping us from merging this into main right now. In fact, I think we should do that ASAP to remove this from nightly dev wheel.

But backport is the controversial one, so we will block the backport from being merged until consensus is reached and also keep the alt PR open but switch its base to v7.0.x instead of main.

So that is what I am gonna do after posting this comment. FYI.

Thanks @pllim - sounds like a good plan.

Technically removal of functionality in a minor release would still run against APE 2, right? But if it's 1 year after the 6.1.0 release, I would not object to that.

But if it's 1 year after the 6.1.0 release, I would not object to that.

Except it was a pending deprecation that in practice never shows.

So, while I support the removal of the bundled JS in the next release (and thus the API change for the behaviour of a couple of kwargs), I would like to see a proper deprecation for show_in_browser and then its removal no earlier than the standard deprecation cycle.
Too many things got broken recently due to hasty removals all the while a mature core library should in fact be more careful and slower with changing API.

Just a note for casual reader looking over this discussion later: This PR does NOT remove show_in_browser, it only removes the bundled libraries so show_in_browser will only work when online or files from the CDN are locally cached.

it only removes the bundled libraries so show_in_browser will only work when online or files from the CDN are locally cached.

In that case, is it possible to throw the deprecation warning only when the method notices the bundled JS is being accessed? 🤔

like so: https://github.com/astropy/astropy/pull/17521/files#diff-130f9ab76367582839ed02de1282547001dbad88994b617171899df5bc1cc316R135 ?

like so

Yes, I think that is sufficient, right? Sorry I missed that in the diff before. So since it has been removed in v7.1 (main), should we backport the deprecation warning like this but to the patched version in #17480 ?

Re: #17521 (comment)

CoCo has discussed this. Here are the votes as of 2024-12-16:

• Patch instead of remove: @pllim @ceb8 @dhomeier
• Abstain: @kelle
• No response: @eteq

So 3/5 (majority) voted for #17480 .

Yes, I think that is sufficient, right? Sorry I missed that in the diff before. So since it has been removed in v7.1 (main), should we backport the deprecation warning like this but to the patched version in #17480 ?

Better late than never at all