Revive Table.show_in_notebook method but with ipydatagrid by pllim · Pull Request #17165 · astropy/astropy

pllim · 2024-10-11T20:47:49Z

Description

This pull request is to address demand to bring back show_in_notebook by un-deprecating it but now use ipydatagrid as suggested in #16702 (comment) .

Fixes #16702

Reverts #15905 a little

Rendered doc: https://astropy--17165.org.readthedocs.build/en/17165/table/index.html

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-10-11T20:48:20Z

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-10-11T20:48:22Z

👋 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?

pllim · 2024-10-11T20:49:58Z


-@pytest.mark.skipif(not HAS_IPYTHON, reason="requires iPython")
+@pytest.mark.skipif(not HAS_IPYDATAGRID, reason="requires ipydatagrid")
+# https://github.com/bqplot/bqplot/issues/1624 and such


xref

DeprecationWarning: Sentinel is not a public part of the traitlets API (from traittypes) bqplot/bqplot#1624

eerovaher · 2024-10-11T21:54:50Z

They way this is implemented right now is a backwards-incompatible change to show_in_notebook(). Maybe it would be cleaner to avoid breaking API by implementing it as a new show_as_datagrid() function?

pllim · 2024-10-11T22:08:22Z

Re: show_as_datagrid

The pro is we are free of API debt. The con is people might not discover it, though I guess we can add that as the alternative in the deprecation warning. Let's see what @taldcroft thinks. Thanks!

astrofrog · 2024-10-12T09:21:50Z

A compromise to avoid any actual errors due to API change would be to deprecate the original keyword arguments unless they are also used for ipydatagrid. We could even translate them into ipydatagrid arguments if possible. But I don't think we should use a different method name.

pllim · 2024-10-12T15:54:22Z

Hmm. I'd rather not have the burden of us keeping track of ipydatagrid changes, especially since they don't seem to have proper API documentation.

taldcroft · 2024-10-13T10:08:23Z

I agree with @astrofrog that we should not introduce a new method name, this just causes more API bloat. Instead, how about introducing a backend kwarg to show_in_notebook. Then we could support multiple backend table viewers, but most importantly for now retain support for backend="astropy" which uses the current built-in (but deprecated) code.

def show_in_notebook(self, *, backend="ipydatagrid", **kwargs):
    method = getattr(self, f"_show_in_notebook_{backend}", None)
    if method is None:
        raise ValueError(f"backend '{backend}' is not supported")
    return method(**kwargs)

This is an API change in that the default backend is ipydatagrid not astropy so existing notebooks using this that specify kwargs from the astropy backend will break. However for the 7.0 release this seems OK to me, we just need to highlight this in the What's New.

eerovaher · 2024-10-14T12:42:11Z

The backend parameter sounds like a good idea. Would it be a good idea to make this mechanism extendible so that downstream developers could implement their own backends?

pllim · 2024-10-14T20:56:55Z

make this mechanism extendible

What kind of mechanism do you have in mind?

pllim · 2024-10-14T22:15:35Z

Re: #17165 (comment)

@taldcroft , but then how do we document all the different combos of kwargs per backend?

pllim · 2024-10-14T22:16:35Z

Also multiple backends are going to bloat up our "optional" dependencies.

taldcroft · 2024-10-15T11:06:27Z

About documentation, maybe put the backend functions into astropy/table/notebook_backends.py, named like the backend. So def astropy(..) and def ipydatagrid(...). Then the show_in_notebook docstring can refer to that module API docs for backend-specific kwargs.

About optional dependency bloat, that's always a thorny question. I don't have a great answer except to say that we shouldn't let slippery-slope arguments derail our progress here. For this particular PR we are just adding one new optional dependency, which is OK. Questions about the slippery-slope can be considered out of scope.

About a mechanism for user defined backends, I thought about it as well. At this point I'm not sure why the user needs to use the show_in_notebook method if they already have their own local code. I would say to wait with this idea until there is a demonstrated need for this functionality. At the very least this is an idea for the future, out of scope in this PR.

eerovaher · 2024-10-15T14:37:11Z

I don't think there's any real need to support user-defined backends in v7.0, but they would be nice to have in the future. When we do want to enable user-defined backends then having a separate astropy.table.notebook_backends namespace to work in should make the implementation reasonably straightforward.

taldcroft · 2024-10-15T15:03:48Z

having a separate astropy.table.notebook_backends namespace to work in should make the implementation reasonably straightforward.

Yup. This could be a registration function, a global dict of available backend functions, and a module-level __getattr__.

pllim · 2024-10-15T19:05:38Z

So... you all want to get rid of notebook support altogether a few months ago. But now want to support all whole slew of backends? That is quite a turn.

taldcroft · 2024-10-15T19:30:09Z

So... you all want to get rid of notebook support altogether a few months ago. But now want to support all whole slew of backends? That is quite a turn.

What we didn't want was maintaining fiddly javascript code that was coupled to external tools like Jupyter. But we listened to users who said that they really do want rich notebook table display and then realized that plugging in a 3rd party tool would satisfy both needs. The evolution to backends is not strictly necessary but seems to neatly allow a bit of back-compatibility (for the deprecated astropy version) while making a new version (ipydatagrid) be the default.

Sometimes life and code is a rollercoaster ride, no? 😄

pllim · 2024-10-15T20:10:17Z

Okay, I think I implemented requested changes. Please re-review. Thanks!

pllim · 2024-10-15T21:51:21Z

Note: I followed the layout from modeling.

taldcroft · 2024-10-16T10:55:43Z

Thanks @pllim! Just a bunch of word-smithing.

taldcroft · 2024-10-16T10:55:58Z

I think this should also get a Whats New.

taldcroft · 2024-10-16T12:19:18Z

One more thing is that the show_in_notebook docstring should provide some hint about finding the backend API docs so users can discover the appropriate kwargs.

pllim · 2024-10-16T18:09:20Z

@taldcroft , I think I implemented all your feedback except #17165 (comment)

docstring should provide some hint about finding the backend API docs so users can discover the appropriate kwargs

Is the "See Also" section not enough? What else do you have in mind?

taldcroft · 2024-10-19T13:20:42Z

@pllim - I think the See also is OK but not sufficient. I made a comment with a suggestion.

pllim · 2024-10-21T20:01:31Z

Please re-review. Thanks! 🙏

taldcroft

Looking great. I did some functional testing and it works as expected. Just a few more suggestions, mostly on docs.

taldcroft · 2024-10-22T10:56:02Z

+    .. note::
+        This function requires an optional dependency, ``ipydatagrid``.
+
+    Parameters


Add something like:

This function creates an ``ipydatagrid.DataGrid`` object by converting the input ``table`` to a ``pandas.DataFrame`` and passing ``**kwargs`` to the constructor. The available ``DataGrid`` options can be seen in a Jupyter notebook with ``help(ipydatagrid.DataGrid)``.

taldcroft · 2024-10-22T10:56:52Z

+    **kwargs : dict, optional
+        Keyword arguments accepted by ``ipydatagrid.DataGrid``.
+
+    """


Add a Returns block.

I didn't because the "classic" backend docstring never had one. Also, when used in practice, the returned object is used directly by the notebook, so it is not something that should concern the user?

The show_in_notebook() function in both cases returns an object that supports rich display in Jupyter notebook (for effectively repr(obj)). There is nothing stopping the user from doing:

dg = table.show_in_notebook() # ... manipulate `dg` somehow with public methods and attributes dg

The historical fact that the return type was not documented is IMO a bug in the original.

but with ipydatagrid. Co-authored-by: Tom Aldcroft <[email protected]>

as taldcroft and eerovaher suggested Expose notebook_backends for API doc

and other minor doc edits. [ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

but the suggestions are incomplete [ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

and code clean-up including fixing test for classic, making ipydatagrid check come first because it is new default and stop referring to self (astropy) as a third person in docstring.

Co-authored-by: Tom Aldcroft <[email protected]>

[ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

pllim · 2024-10-22T16:01:21Z

Thanks for catching all the details, @taldcroft . I think I have addressed them all now. Please rerereview.

taldcroft

Thanks @pllim this is great work and a nice example of astropy listening to users! I especially liked the What's New text and the image.

pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Oct 11, 2024

pllim added this to the v7.0.0 milestone Oct 11, 2024

github-actions Bot added table Docs utils installation labels Oct 11, 2024

pllim force-pushed the show-me-the-notebook branch from 2a67a04 to 5fd7245 Compare October 11, 2024 20:48

pllim commented Oct 11, 2024

View reviewed changes

pllim mentioned this pull request Oct 11, 2024

Support showing interactive tables in Jupyter Notebooks (again!) #16702

Closed

pllim marked this pull request as ready for review October 11, 2024 21:09

pllim requested a review from taldcroft as a code owner October 11, 2024 21:09

pllim force-pushed the show-me-the-notebook branch from 5fd7245 to f4cabb7 Compare October 15, 2024 20:09

pllim force-pushed the show-me-the-notebook branch from 998d353 to 9df83e3 Compare October 15, 2024 21:48

pllim commented Oct 15, 2024

View reviewed changes

Comment thread docs/table/ref_api.rst

pllim Oct 15, 2024

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

Note: I followed the layout from modeling.

taldcroft requested changes Oct 16, 2024

View reviewed changes

pllim added the whatsnew-needed label Oct 16, 2024

pllim force-pushed the show-me-the-notebook branch from 415eb3b to dea1980 Compare October 16, 2024 18:03

taldcroft reviewed Oct 19, 2024

View reviewed changes

Comment thread astropy/table/table.py Resolved

pllim force-pushed the show-me-the-notebook branch from dea1980 to 0c8e447 Compare October 21, 2024 19:41

taldcroft mentioned this pull request Oct 22, 2024

Add API docs (at least for DataGrid) jupyter-widgets/ipydatagrid#552

Open

taldcroft reviewed Oct 22, 2024

View reviewed changes

pllim and others added 9 commits October 22, 2024 11:24

Revive Table.show_in_notebook method

8a36d3c

but with ipydatagrid. Co-authored-by: Tom Aldcroft <[email protected]>

Implement notebook backends

10d2007

as taldcroft and eerovaher suggested Expose notebook_backends for API doc

DOC: Replace IPython with Jupyter

c95c967

and other minor doc edits. [ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

Backend default is ipydatagrid

a8c23d2

but the suggestions are incomplete [ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

Add What's New entry

1c2dde5

and code clean-up including fixing test for classic, making ipydatagrid check come first because it is new default and stop referring to self (astropy) as a third person in docstring.

DOC: Clarify backend kwargs

0a4df3d

Co-authored-by: Tom Aldcroft <[email protected]>

Apply suggestions from code review

b9bf579

[ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

Change deprecation message for classic

bdb68fd

[ci skip] Co-authored-by: Tom Aldcroft <[email protected]>

Add Returns and test also requires pandas

1601476

pllim force-pushed the show-me-the-notebook branch from 23f6e11 to 1601476 Compare October 22, 2024 15:55

taldcroft approved these changes Oct 22, 2024

View reviewed changes

pllim merged commit 77da8fe into astropy:main Oct 22, 2024

pllim deleted the show-me-the-notebook branch October 22, 2024 17:26

pllim mentioned this pull request Oct 28, 2024

Clearer error when show_in_notebook is missing optional dependencies #17268

Merged

1 task

Conversation

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

Uh oh!

Description

Uh oh!

github-actions Bot commented Oct 11, 2024

Uh oh!

github-actions Bot commented Oct 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Oct 11, 2024

Uh oh!

pllim commented Oct 11, 2024

Uh oh!

astrofrog commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Oct 12, 2024

Uh oh!

taldcroft commented Oct 13, 2024

Uh oh!

eerovaher commented Oct 14, 2024

Uh oh!

pllim commented Oct 14, 2024

Uh oh!

pllim commented Oct 14, 2024

Uh oh!

pllim commented Oct 14, 2024

Uh oh!

taldcroft commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerovaher commented Oct 15, 2024

Uh oh!

taldcroft commented Oct 15, 2024

Uh oh!

pllim commented Oct 15, 2024

Uh oh!

taldcroft commented Oct 15, 2024

Uh oh!

pllim commented Oct 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft commented Oct 16, 2024

Uh oh!

taldcroft commented Oct 16, 2024

Uh oh!

taldcroft commented Oct 16, 2024

Uh oh!

pllim commented Oct 16, 2024

Uh oh!

Uh oh!

taldcroft commented Oct 19, 2024

Uh oh!

pllim commented Oct 21, 2024

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 11, 2024 •

edited

Loading

astrofrog commented Oct 12, 2024 •

edited

Loading

taldcroft commented Oct 15, 2024 •

edited

Loading