ibis
ibis copied to clipboard
feat(ux): add Table and Column.preview()
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:
- 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.
- 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.
- Should
.preview()
return arich.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 likedef 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 toprint()
it, otherwise I get the raw output with ANSI codes:
@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?
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)
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.
yeah, easy enough to expose/figure it out later if you get a feature request, but hard to take back :)
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.
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.
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
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
OK, there was an inconsistency between the __interactive_rich_console__
implementations in Column vs Table. Now they are unified, along with Scalars
@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.
@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
@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 I haven't forgotten :) We'll definitely get to it for 9.0, I can't promise anything this week though :)
@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 I just DM'd you on Zulip, was hoping I could pair with you on this to get it over the finish line.
@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.
I think option A until we know more about how people are going to use it.
Docs preview: https://pr-7915-45ecf40b91269567bcdb45b30356b6b0c08814f8--ibis-quarto.netlify.app
OK @cpcloud, once you mark as resolved then this should be good to merge!