Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ux): add Table and Column.preview() #7915

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Jan 5, 2024

redo of #7408 targeting main instead of master

fixes #7172

I wasn't sure how testing this should work so I didn't add any tests. I would really appreciate it if someone more familiar could pick this up and finish it off, I think I've gotten a lot of it out of the way.

Hopefully I've implemented this is such a way that it is testable.

Things I'm not sure about:

  1. EDIT: I think this is fine to just keep them opaque as fmt_kwargs. adding **fmt_kwargs all over pretty.py. I didn't really want all the boilerplate that would be required if I was explicit with each kwarg. Possibly could pass around a container object such as options.Repr instead.
  2. I encapsulated some logic into pretty.pretty_repr() because I thought that was going to be necessary for the functional change. It ended up not being needed, but I still think it is a good refactor to tie up some disparate ends. I can revert this though if you want.
  3. EDIT: we decided this should just print. Should .preview() return a rich.Table for the caller/framework to render, or should it print to stdout directly? (ie the same difference as ibis.to_sql() vs ibis.show_sql())
    I might argue for a kwarg like def preview(..., file: io.StringIO | None = None), so the default is to just print to stdout.
    This is because I imagine this method is mostly going to get used in notebooks or debuggers. For example, in the vscode debugger, if I want to see an ibis table, I have to print() it, otherwise I get the raw output with ANSI codes:
image image

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM, modulo removal of the show_types argument and getting CI to pass.

ibis/expr/types/generic.py Outdated Show resolved Hide resolved
@cpcloud cpcloud requested a review from jcrist January 28, 2024 13:17
@cpcloud cpcloud added the ux User experience related issues label Jan 28, 2024
@cpcloud
Copy link
Member

cpcloud commented Feb 5, 2024

@NickCrews Regarding returning versus printing, I vote for printing.

It seems quite inconvenient to have to write print(t.preview()).

Are there any places where automatically calling print won't work?

@NickCrews
Copy link
Contributor Author

NickCrews commented Feb 6, 2024

I agree I want to avoid the print()

I personally have no use for getting a rich table, but I could see someone else wanting a .to_rich() method? Could add that as another public method that just returns a rich table (and does no shortening with .head()).

Then preview could be (EDIT: this won't work because when we make the rich table we need to know if the rows have been cut off, so we can add ellipses if so. So we need to pass the whole table to the function that makes the rich table)

if max_rows is None:
    max_rows = ibis.options.etc
head = t.head(max_rows)
rich_table = head.to_rich(max_len, max_depth, etc)
print(rich_table)

@NickCrews NickCrews marked this pull request as draft February 6, 2024 18:50
@cpcloud
Copy link
Member

cpcloud commented Feb 6, 2024

Can you make to_rich private for now (as _to_rich)? I'd rather not expose something we don't know is useful beyond internals.

@NickCrews
Copy link
Contributor Author

yeah, easy enough to expose/figure it out later if you get a feature request, but hard to take back :)

ibis/expr/types/pretty.py Outdated Show resolved Hide resolved
@NickCrews
Copy link
Contributor Author

OK, not sure why the widths of the tables are changing in the doctests, I don't see what is different. But I can dig into that once the other questions I have are addressed.

@NickCrews NickCrews force-pushed the table-preview branch 4 times, most recently from a13c572 to a0d2d09 Compare February 20, 2024 05:22
@NickCrews
Copy link
Contributor Author

for myself: can repro the doctest failure with pytest --doctest-modules ibis/expr/types/maps.py

@cpcloud so now the only test failing are the doctests. I'm not sure exactly what is causing the change in behavior there. Want me/us to track that down and keep the behavior consistent? Or just update the docstrings to the new behavior? I sort of like the new behavior more (it leads to being more compact)

I also need to add tests for the actual .preview method, but let's get the implementation settled to avoid churn with me rewriting the test.

@NickCrews
Copy link
Contributor Author

NickCrews commented Feb 29, 2024

OK the failing doctests are only for nested datatypes. There is some conditional logic inside to_rich_table(), which I think is the culprit

        if show_types and not isinstance(dtype, (dt.Struct, dt.Map, dt.Array)):
            # Don't truncate non-nested dtypes
            min_width = max(min_width, len(dtype_str))

investigating this now

@NickCrews
Copy link
Contributor Author

no, that's not it.

On main, to_rich_table is getting called with width=153 (or the width of the rich console) found in interactive_rich_console.

on this branch, to_rich_table is getting called with width=None.

Looking into this more

@NickCrews NickCrews force-pushed the table-preview branch 2 times, most recently from 5829c1e to b33b8d9 Compare February 29, 2024 22:31
@NickCrews
Copy link
Contributor Author

OK, there was an inconsistency between the __interactive_rich_console__ implementations in Column vs Table. Now they are unified, along with Scalars

@NickCrews NickCrews force-pushed the table-preview branch 2 times, most recently from 4879a92 to a0b2852 Compare March 1, 2024 00:01
@@ -219,21 +262,76 @@ def isnull(x):
return out, min_width, max_width


def format_dtype(dtype):
max_string = ibis.options.repr.interactive.max_string
def format_dtype(dtype, max_string: int) -> Text:
Copy link
Contributor Author

@NickCrews NickCrews Mar 1, 2024

Choose a reason for hiding this comment

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

this is a small change. Before, the passed in max_string was ignored, and we only used the one in options.repr.interactive.

So t.island.preview(max_string=5) now gives:

        ┏━━━━━━━━┓
        ┃ island ┃
        ┡━━━━━━━━┩
        │ stri…  │
        ├────────┤
        │ Torg…  │
        │ Torg…  │
        │ Torg…  │
        │ …      │
        └────────┘

without this, you get the full string

I think this is better, especially when you have a really long dtype from some complex nested dtype. Here it's a bit silly.

@NickCrews
Copy link
Contributor Author

@cpcloud OK now I got this all fixed up! Can you review this now? The implementation has changed decently since the last time you looked.

This is doing both a refactor and a new feature at the same time. If this is too much, I can go through and split this into two commits, first the refactor and then the feature.

def _format_nested(values):
interactive = ibis.options.repr.interactive

def pretty_repr(expr: ibis.Expr) -> str:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function feels similar to __interactive_rich_console__, in a followup they might want to get unified.

@NickCrews NickCrews marked this pull request as ready for review March 1, 2024 04:09
@NickCrews
Copy link
Contributor Author

@cpcloud sorry to bug you, could this get a review? I can fix the rebase error once I get a +1 on the general approach

@NickCrews
Copy link
Contributor Author

@lostmygithubaccount could you consider adding this to the 9.0 milestone so it doesn't get forgotten about? It looks like this has gotten lost in phillip's inbox since this is now on page 2 of the PRs :)

@cpcloud
Copy link
Member

cpcloud commented Mar 19, 2024

@NickCrews I haven't forgotten :) We'll definitely get to it for 9.0, I can't promise anything this week though :)

@NickCrews
Copy link
Contributor Author

@cpcloud is this still targeted for 9.0.0? It's not added to that milestone and I'm worried it will get left behind

@cpcloud
Copy link
Member

cpcloud commented Apr 4, 2024

@NickCrews I just DM'd you on Zulip, was hoping I could pair with you on this to get it over the finish line.

@NickCrews NickCrews force-pushed the table-preview branch 2 times, most recently from d3d50b8 to 9ef131f Compare April 10, 2024 00:50
@NickCrews
Copy link
Contributor Author

@cpcloud what do you want to do about this failing doctest? Basically, .preview() is returning a rich.Table. In a jupyter notebook, or ipython, this works and you get the full table output (I think because Table has a _repr_mimebundle method). In the vanilla python REPL though, you just get <rich.table.Table object at 0x12b961450>. So either we can

  • A: keep it as is. Sorry vanilla REPL people you get a bad experience, but everyone else is OK. Add a #ignore pragma to this doctest
  • B: convert that Table to a str, so all environments get the full render. Sorry to anyone who wants to do anything interesting with the Table
  • C: add a flag format: Literal["str", "rich"] = "str". By default do A, but if someone really wants, they can get the rich Table.

@cpcloud
Copy link
Member

cpcloud commented Apr 10, 2024

I think option A until we know more about how people are going to use it.

- unifies __interactive_rich_console__()
from Scalar, Column, and Relation into the parent Expr's __rich_console__(). This calls to_rich(),
which is reposnsible for turning an Expr into a rich object.
Now all implementations have the console.is_jupyter checks,
before only the Table did.
- propagates repr options downward through all the formatters in pretty.py,
  so you can pass hardcoded options instead of pulling from ibis.options.repr.interactive
- moves the check for ibis.options.repr.show_variables inside
  format.pretty(), the only place where it is used.
@NickCrews NickCrews added the docs-preview Add this label to trigger a docs preview label Apr 11, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Apr 11, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Apr 11, 2024

@NickCrews NickCrews enabled auto-merge (squash) April 11, 2024 20:29
@NickCrews
Copy link
Contributor Author

OK @cpcloud, once you mark as resolved then this should be good to merge!

@NickCrews NickCrews merged commit 1c03ad0 into ibis-project:main Apr 12, 2024
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: ux: inline way to adjust interactive config
2 participants