ibis icon indicating copy to clipboard operation
ibis copied to clipboard

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

Open NickCrews opened this issue 1 year ago • 6 comments

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

NickCrews avatar Jan 05 '24 06:01 NickCrews

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

cpcloud avatar Feb 05 '24 11:02 cpcloud

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 avatar Feb 06 '24 17:02 NickCrews

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.

cpcloud avatar Feb 06 '24 19:02 cpcloud

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

NickCrews avatar Feb 06 '24 19:02 NickCrews

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 avatar Feb 06 '24 20:02 NickCrews

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 avatar Feb 20 '24 05:02 NickCrews

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 avatar Feb 29 '24 21:02 NickCrews

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 avatar Feb 29 '24 21:02 NickCrews

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

NickCrews avatar Feb 29 '24 22:02 NickCrews

@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.

NickCrews avatar Mar 01 '24 00:03 NickCrews

@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 avatar Mar 13 '24 21:03 NickCrews

@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 :)

NickCrews avatar Mar 19 '24 19:03 NickCrews

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

cpcloud avatar Mar 19 '24 19:03 cpcloud

@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

NickCrews avatar Apr 04 '24 18:04 NickCrews

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

cpcloud avatar Apr 04 '24 18:04 cpcloud

@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.

NickCrews avatar Apr 10 '24 01:04 NickCrews

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

cpcloud avatar Apr 10 '24 14:04 cpcloud

Docs preview: https://pr-7915-45ecf40b91269567bcdb45b30356b6b0c08814f8--ibis-quarto.netlify.app

ibis-docs-bot[bot] avatar Apr 11 '24 19:04 ibis-docs-bot[bot]

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

NickCrews avatar Apr 11 '24 20:04 NickCrews