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

[hail] teach tables how to HTML #5666

Merged
merged 14 commits into from Mar 28, 2019

Conversation

Projects
None yet
2 participants
@danking
Copy link
Collaborator

commented Mar 21, 2019

Take a look at the docs for IPython.display.display.

I preserve the user's ability to specify a custom handler. The handler is no longer given a string but an object that has a sensible __str__ and __repr__. Moreover, this object has a _repr_html_ which Jupyter uses to display an HTML table. Detecting what frontend is being run is done by IPython.display.display.

I use the _Show shim class to avoid having tables themselves print as HTML.

I also check for terminal size and use that to pick n and width.

Should _hl_repr live here?

Resolves #5663, #2847

danking added some commits Mar 21, 2019

@@ -279,7 +279,7 @@ task testPython(type: Exec, dependsOn: shadowJar) {
'--color=no',
'-r a',
'--html=build/reports/pytest.html',
'--self-contained-html',
'--self-contained-html', '-vv', '--maxfail=1',

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 21, 2019

Collaborator

disable?

t = t.flatten()
fields = list(t.row)

formatted_t = t.select(**{k: hl_format(v) for (k, v) in t.row.items()})

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 21, 2019

Collaborator

This won't work - it'll try to call hl_format on expressions. hl_format needs to be called on strings after the take localizes them to python

This comment has been minimized.

Copy link
@danking

danking Mar 22, 2019

Author Collaborator

This isn't hl.format, its hl_format, which is defined above to escape after calling Table._hl_repr. This is basically the same code as what show previously did except I removed truncation and added cgi.escape.

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 22, 2019

Collaborator

wait, but isn't this calling cgi.escape inside of a TableMapRows (select) above? How does that work?

This comment has been minimized.

Copy link
@danking

This comment has been minimized.

Copy link
@danking

danking Mar 22, 2019

Author Collaborator
import html
print(html.escape(hl.str("tlocus<GRCh37>"))._ir)
(Apply replace (Apply replace (Apply replace (Apply replace (Apply replace (Str "tlocus<GRCh37>") (Str "&") (Str "&amp;")) (Str "<") (Str "&lt;")) (Str ">") (Str "&gt;")) (Str "\"") (Str "&quot;")) (Str "'") (Str "&#x27;"))

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 22, 2019

Collaborator

I don't really like this -- I think there's nothing that prevents the cgi or html implementation from changing and breaking this, and Hail string manipulation is also extremely slow right now.

Can we just call hl_format on each string coming out in line 1367?


if has_more:
n_rows = len(rows)
s += f"<p>showing top { n_rows } { 'row' if n_rows == 1 else 'rows' }</p>\n"

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 21, 2019

Collaborator

we have a plural utility function in hail.utils.misc

This comment has been minimized.

Copy link
@danking

danking Mar 22, 2019

Author Collaborator

fixed

handler(self._show(n, width, truncate, types))
if n is None or width is None:
import shutil
(columns, lines) = shutil.get_terminal_size((80, 10))

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 21, 2019

Collaborator

very nice.

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 22, 2019

e.g. Jupyter. Looks like Jupyter also supports terminal size, and it chooses 40 in this case, which means Jupyter told us 50 lines, which seems a bit much. In reality you can fit about 35 HTML table lines in that browser window including the Jupiter header (which eats about 5 lines).

Screen Shot 2019-03-22 at 12 04 39 PM

@danking

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 22, 2019

IPython
Screen Shot 2019-03-22 at 12 09 18 PM

wip

addressed

@tpoterba
Copy link
Collaborator

left a comment

move hl_format

@danking danking force-pushed the danking:table-html branch from 950cdc5 to 1f9bb99 Mar 22, 2019

danking added some commits Mar 22, 2019

addressed

@tpoterba

This comment has been minimized.

Copy link
Collaborator

commented Mar 22, 2019

doctest failure from show() changing the width, it seems

s = "{" + hl.delimit(hl.map(lambda x: Table._hl_repr(x[0]) + ":" + Table._hl_repr(x[1]), hl.array(v)), ",") + "}"
elif v.dtype == hl.tstr:
s = hl.str('"') + hl.expr.functions._escape_string(v) + '"'
elif isinstance(v.dtype, (hl.tstruct, hl.tarray)):

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 22, 2019

Collaborator

you didn't change this code, but this should be ttuple not tarray

danking added some commits Mar 22, 2019

@tpoterba
Copy link
Collaborator

left a comment

So awesome.

elif v.dtype == hl.tstr:
s = hl.str('"') + hl.expr.functions._escape_string(v) + '"'
elif isinstance(v.dtype, (hl.tstruct, hl.ttuple)):
s = "(" + hl.delimit([Table._hl_repr(v[i]) for i in range(len(v))], ",") + ")"

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 25, 2019

Collaborator

needs to check for empty array - that can't be type-imputed

This comment has been minimized.

Copy link
@tpoterba

tpoterba Mar 25, 2019

Collaborator

(this is your test failure)

This comment has been minimized.

Copy link
@danking

danking Mar 27, 2019

Author Collaborator

le sigh. fixed.

@tpoterba
Copy link
Collaborator

left a comment

boom!

@danking danking merged commit c1d08a7 into hail-is:master Mar 28, 2019

1 check passed

hail-ci-0-1 successful build
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.