Update bundled datatables with current version (v7.0.x) by hamogu · Pull Request #17480 · astropy/astropy

hamogu · 2024-11-29T12:25:45Z

Description

While we are deprecating the bundeled datatables (#16067) to avoid having to maintain js code, as long as we do we should update regularly to avoid any security holes that might exist in older code.

An alternative to this PR is #17521.

Change the base from main to v7.0.x. GitHub couldn't do this automagically from the web UI.
Address comments in this PR and Remove js code bundled with Astropy for DataTables #17521 (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-11-29T12:26:09Z

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.

hamogu · 2024-12-05T14:31:34Z

Note that the git pre-commit failure should be ignored. It warns that I added a large file > 300 kb - but there is nothing we can do about it, that's just the size of the external library that we bundle.

eerovaher · 2024-12-05T14:55:10Z

You need to edit the pre-commit configuration:

astropy/.pre-commit-config.yaml

Lines 9 to 14 in 4a55b10

    
                 - id: check-added-large-files 
        
                   args: ["--enforce-all", "--maxkb=300"] 
        
                   exclude: "^(\ 
        
                       cextern/wcslib/C/flexed/.*|\ 
        
                       CHANGES.rst|\ 
        
                       astropy/extern/jquery/data/js/jquery.dataTables.js|\

hamogu · 2024-12-06T20:45:33Z

I checked the display manually and for the tables that I tried it works. That's not surprising: The API changes in DataTables from version 1 to 2 are small and and we don't do any interactions with other code once it's displayed (editing cells, feeding back cell values into some other code), which is where the API changes are.

taldcroft · 2024-12-07T10:22:00Z

I have forgotten why astropy is bundling these libraries at all. Aren't the CDN versions sufficient?

I'm hesitant to add nearly 0.5 Mb of churn to the astropy git repo for some large data files that only support a deprecated feature.

taldcroft · 2024-12-07T10:38:14Z

OK, now I see the use_local_files option in the JSViewer class, which is deprecated (or pending?).

The other thing that is surprising me is that using tbl.show_in_browser() (with or without jsviewer=True) does not show any pending deprecation message. I'm not sure if that is intended behavior but it definitely fails to provide users with any notice of the pending deprecation.

taldcroft · 2024-12-07T10:40:19Z

For such a rarely used feature, perhaps we should consider changing the default of use_local_files from True to False for the next feature release, and leave the local files the same. Nothing will break for users unless they are also working without internet.

pllim

Thanks! Just some minor comments, otherwise LGTM.

Years ago, we decided to bundle DataTable (and jQuery that is depends on) with astropy. The sole purpose of this package is to display astropy table in the webbrowser and the js code was bundled to allow that functionality without being online. However, I now view this as a bug - we are not equipped to stay on top of potential js security and performance concerns. Online access is available nearly anywhere and talking to a number of users has shown that this feature is useful, but not absolutely essential for the rare offline work. Also, browsers cache libraries downloaded from a CDN, so people who use this feature frequently in fact will be able to use it offline, because their browsers will have cached the required js and css files already anyway. closes astropy#17480

pllim · 2024-12-17T15:17:10Z

pre-commit.ci autofix

pllim · 2024-12-17T15:32:21Z

I guess the warning is working! But it needs to be handled. Also, does the text need updating?

AstropyDeprecationWarning: use_local_files is deprecated and will be ignored or removed in astropy 7.1

Also no need for backticks in the message.

hamogu · 2024-12-17T15:36:34Z

I thought the Coco took the PR that will ignore use_local_files and it is merged into main, which means that this keyword will be ignored from 7.1 on, so that is what I should users warn about? I'm sorry if I misunderstood please let me know what you would like the warning to say to users.

pllim · 2024-12-17T15:39:35Z

"or removed" is not necessary?

hamogu · 2024-12-17T15:44:43Z

Not in 7.1, where we just ignore it, but I assume we'll remove it eventually so we want to give users a heads up that, if they modify their code anyway, that's what's going to happen at some point?

In particular, this adds a more visible deprecation warning. Co-authored by: P. L. Lim <[email protected]>

pllim · 2024-12-17T16:30:43Z

        self._use_local_files = use_local_files
+        if use_local_files:
+            warn(
+                "`use_local_files` is deprecated and will be ignored or removed in astropy 7.1.",


Suggested change

"`use_local_files` is deprecated and will be ignored or removed in astropy 7.1.",

"use_local_files is deprecated and will be ignored in astropy 7.1 and removed in the future.",

pllim · 2024-12-17T16:31:59Z

-        tmpfile, format="jsviewer", table_id="test", jskwargs={"use_local_files": True}
-    )
+    with pytest.warns(
+        AstropyDeprecationWarning, match="`use_local_files` is deprecated"


Suggested change

AstropyDeprecationWarning, match="`use_local_files` is deprecated"

AstropyDeprecationWarning, match="use_local_files is deprecated"

pllim · 2024-12-17T16:33:14Z

Also need an additional change log about deprecating use_local_files ?

Technically not sure if we supposed to have "api" section in backport branch though... Hmm, where to put it?

Let's just do it.

pllim

LGTM, thanks! But I want to give Brigitta and Simon a chance to review. 🤞

pllim · 2024-12-18T23:47:03Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

saimn · 2025-02-06T18:16:58Z

I lost track, what is the status with this one ? If it should go in 7.0.1, is it ready to go ?

pllim · 2025-02-06T18:40:45Z

I did approve it. I think there was a concern about changing behavior in bugfix release but this is only an issue for a very niched use case, so I think the benefit outweighs the risk. So let's get it in!

saimn · 2025-02-06T19:30:53Z

Ok let's do it then.

hamogu requested a review from taldcroft as a code owner November 29, 2024 12:25

github-actions Bot added table config external PRs and issues related to external packages vendored with Astropy (astropy.extern) labels Nov 29, 2024

hamogu force-pushed the update_datatables branch from 8d889ec to fe20821 Compare December 5, 2024 14:29

hamogu requested a review from pllim December 5, 2024 14:29

hamogu added this to the v7.0.1 milestone Dec 5, 2024

hamogu force-pushed the update_datatables branch from fe20821 to 22409a4 Compare December 6, 2024 16:19

hamogu requested review from WilliamJamieson and nstarman as code owners December 6, 2024 16:19

hamogu mentioned this pull request Dec 6, 2024

table show_in_browser() broken #17512

Closed

hamogu force-pushed the update_datatables branch from 22409a4 to 45573f0 Compare December 6, 2024 20:43

hamogu force-pushed the update_datatables branch from 45573f0 to 9e6e606 Compare December 6, 2024 20:47

hamogu mentioned this pull request Dec 7, 2024

Remove js code bundled with Astropy for DataTables #17521

Merged

1 task

pllim added the backport-v7.0.x label Dec 9, 2024

pllim reviewed Dec 9, 2024

View reviewed changes

Comment thread .pre-commit-config.yaml Resolved

Comment thread docs/changes/17480.other.rst Resolved

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

pllim closed this in 228be19 Dec 13, 2024

pllim reopened this Dec 13, 2024

pllim changed the base branch from main to v7.0.x December 13, 2024 14:50

pllim reopened this Dec 17, 2024

Address review comments

7a95210

In particular, this adds a more visible deprecation warning. Co-authored by: P. L. Lim <[email protected]>

hamogu force-pushed the update_datatables branch from ef6e99d to 7a95210 Compare December 17, 2024 15:47

pllim reviewed Dec 17, 2024

View reviewed changes

Resolve comments from PR review

209533d

pllim approved these changes Dec 18, 2024

View reviewed changes

[pre-commit.ci] auto fixes from pre-commit.com hooks

316c621

for more information, see https://pre-commit.ci

pllim requested review from bsipocz and removed request for a team, WilliamJamieson, astrofrog, larrybradley, matteobachetti, mcara and nstarman December 18, 2024 23:48

pllim removed the needs-discussion label Dec 18, 2024

hamogu mentioned this pull request Jan 31, 2025

Cycle 4 funding: Moritz io.ascii astropy/astropy-project#405

Closed

saimn merged commit 7a528db into astropy:v7.0.x Feb 6, 2025

	"`use_local_files` is deprecated and will be ignored or removed in astropy 7.1.",
	"use_local_files is deprecated and will be ignored in astropy 7.1 and removed in the future.",

	AstropyDeprecationWarning, match="`use_local_files` is deprecated"
	AstropyDeprecationWarning, match="use_local_files is deprecated"

Conversation

hamogu commented Nov 29, 2024 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Nov 29, 2024 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamogu commented Dec 5, 2024

Uh oh!

eerovaher commented Dec 5, 2024

Uh oh!

hamogu commented Dec 6, 2024

Uh oh!

taldcroft commented Dec 7, 2024

Uh oh!

taldcroft commented Dec 7, 2024

Uh oh!

taldcroft commented Dec 7, 2024

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

pllim commented Dec 17, 2024

Uh oh!

pllim commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamogu commented Dec 17, 2024

Uh oh!

pllim commented Dec 17, 2024

Uh oh!

hamogu commented Dec 17, 2024

Uh oh!

pllim Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

hamogu Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 18, 2024

Uh oh!

saimn commented Feb 6, 2025

Uh oh!

pllim commented Feb 6, 2025

Uh oh!

saimn commented Feb 6, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hamogu commented Nov 29, 2024 •

edited by pllim

Loading

github-actions Bot commented Nov 29, 2024 •

edited by taldcroft

Loading

pllim commented Dec 17, 2024 •

edited

Loading