Html repr for pipeline #78

jokasimr · 2023-12-01T12:27:37Z

Fixes #70

SimonHeybrock · 2023-12-04T07:48:14Z

src/sciline/pipeline.py

+def _kind_of_provider(p: Callable[..., Any]) -> str:
+    if _qualname(p) == f'{_qualname(Pipeline.__setitem__)}.<locals>.<lambda>':
+        return 'parameter'
+    if _qualname(p) == f'{_qualname(Pipeline.set_param_table)}.<locals>.<lambda>':
+        return 'table'
+    return 'function'


Can we reuse this code in the visualization module?

SimonHeybrock · 2023-12-04T07:51:56Z

src/sciline/pipeline.py

@@ -750,3 +764,52 @@ def bind_and_call(
        if not return_tuple:
            return results[0]
        return results
+
+    def _repr_html_(self) -> str:


Should we adapt some of the docs notebooks to make use of the repr?

jokasimr · 2023-12-08T15:59:20Z

Example based on one of the tutorials in the sciline docs

jokasimr · 2023-12-08T16:06:14Z

This is what is looks like for the SANS2D workflow in esssans:

jokasimr · 2023-12-08T16:15:14Z

Right now the "Providers" table in its current shape doesn't seem very useful. The docs column doesn't really describe what the provider represents and that is what I think we would like to have in the table. The satisfied column is not very useful either because it is ? as soon as the inputs depends on a TypeVar. The satisfied column also doesn't handle ParameterTables well, it will display that a provider is unsatisfied if the requirements come from a parameter table even if the parameter table is available.

nvaytet · 2023-12-11T09:02:37Z

I think the repr is too large, it should be a much more compact table, a bit like the html repr of a scipp data array.
Vertical space is very valuable in jupyter notebooks, and scrolling past large outputs can become annoying.
If you want to include the descriptions from the docstrings, I would at least recommend using a show/hide mechanism.

SimonHeybrock · 2023-12-11T09:16:57Z

I think the repr is too large, it should be a much more compact table, a bit like the html repr of a scipp data array. Vertical space is very valuable in jupyter notebooks, and scrolling past large outputs can become annoying. If you want to include the descriptions from the docstrings, I would at least recommend using a show/hide mechanism.

Agree. Another option is to show the docstring in a tooltip when hovering the mouse.

We also cannot use so much horizontal space, we should show params and providers in a single table, I think?

We also should list the provider names, since this can take different values depending on how a pipeline is put together.

nvaytet · 2023-12-11T09:28:21Z

Wouldn't the output of visualize be a better repr? What was the reason we wanted to have a table?

jokasimr · 2023-12-11T09:37:29Z

I think the motives for adding a html repr for the pipeline were:

having a more compact description
listing information not avaliable in the graphviz, values of parameters etc
listing the domain types that can be produced by the pipeline
to be able to display a pipeline without selecting a particular result
for very large pipelines inspecting the graph can become cumbersome
debugging incomplete pipelines with missing parts

Are there other?

jokasimr · 2023-12-11T10:36:53Z

We also cannot use so much horizontal space, we should show params and providers in a single table, I think?

Sure we can do that but then we will use more vertical space. What's the problem with using the horizontal space?

jokasimr · 2023-12-11T10:41:14Z

New version, docstring removed, added name and source of provider function instead

SimonHeybrock · 2023-12-11T10:55:36Z

Wouldn't the output of visualize be a better repr? What was the reason we wanted to have a table?

I think it serves two very different purposes. Compare, e.g., also Dask's _repr_html_ vs. visualize.

SimonHeybrock · 2023-12-11T10:56:05Z

We also cannot use so much horizontal space, we should show params and providers in a single table, I think?

Sure we can do that but then we will use more vertical space. What's the problem with using the horizontal space?

For example that is does not fit into the docs.

SimonHeybrock · 2023-12-11T10:57:34Z

We also cannot use so much horizontal space, we should show params and providers in a single table, I think?

Sure we can do that but then we will use more vertical space. What's the problem with using the horizontal space?

I am not even sure we want to split providers from params: In different configs of a pipeline we might get the same type from either a provider or a param. Have you considering having everything in a single table, alphabetically?

jokasimr · 2023-12-11T12:05:54Z

We also cannot use so much horizontal space, we should show params and providers in a single table, I think?

Sure we can do that but then we will use more vertical space. What's the problem with using the horizontal space?

I am not even sure we want to split providers from params: In different configs of a pipeline we might get the same type from either a provider or a param. Have you considering having everything in a single table, alphabetically?

That makes sense. I split them because some columns don't map well to parameters that have values, and vice versa for providers that are functions. For example the "Value" column doesn't really make sense for providers that are functions, and "Docs" or "Source" don't really exist for providers that are just values.

But I think you're right, it is better to put them all in same table and just don't put anything in the column if the kind of the provider doesn't have that information.

jokasimr · 2023-12-13T16:02:21Z

Here's a new version.
The satisfied column now displays all type var combinations that are possible (based on the constraints of the type var) and highlights the ones that are satisfied.

It uses quite a lot of space, but on the other hand it is quite useful to be able to tell quickly what providers exist and are satisfied. One option is to make the list of concrete providers collapsed by default.

The value column uses the scipp ._repr_html_() method.

SimonHeybrock · 2023-12-14T09:21:10Z

The satisfied column now displays all type var combinations that are possible (based on the constraints of the type var) and highlights the ones that are satisfied.

It uses quite a lot of space, but on the other hand it is quite useful to be able to tell quickly what providers exist and are satisfied. One option is to make the list of concrete providers collapsed by default.

I think I'd keep things simple for now and simply remove the satisfied column.

nvaytet · 2023-12-15T10:29:06Z

src/sciline/pipeline.py

+            {chr(10).join((f"""
+              <tr>
+                <th scope="row">{pretty_provider_name(p)}</th>
+                <td scope="row">{provider_source(p)}</th>


I'm getting some strange values for the paths in the source dropdowns:

Can we have information of package/module/class it's coming from?
Instead of the source file and number of line.

Yes package/module/function_name makes more sense than file and line number. I'll see if I can fix that.

nvaytet · 2023-12-15T10:30:07Z

src/sciline/pipeline.py

+            <tbody>
+            {chr(10).join((f"""
+              <tr>
+                <th scope="row">{pretty_provider_name(p)}</th>


Do we really need to have the names in bold?

No not really

YooSunYoung · 2023-12-15T15:04:37Z

src/sciline/pipeline.py

+            return f'{p["value"]}'
+
+        return f'''
+        <div style="display: flex; column-gap: 1em; align-items: flex-start;">


Should we maybe use template instead of f-string here...?
Then it'll be much easier to update/fix them later.
Llike for the data group html wrapper: https://github.com/scipp/scipp/blob/main/src/scipp/visualization/formatting_datagroup_html.py

Or maybe later if we want to add more columns...!

I agree the code needs a clean up. I'll collect the inline styles and remove excessive duplication and nested f-strings etc. I'm not sure that the refactor will involve templates, I'll look at the datagroup code but it might be overkill here.

Maybe you can also move this to a different file to not grow pipeline.py too much?

SimonHeybrock · 2023-12-20T05:49:28Z

src/sciline/pipeline.py

+        providers = groupby(
+            lambda p: p.kind,
+            (
+                ProviderDisplayData(
+                    origin,
+                    args,
+                    k := _kind_of_provider(value),
+                    value() if k != 'function' else value,
+                )
+                for origin, args, value in chain(
+                    providers_with_parameters, providers_without_parameters
+                )
+            ),
+        )


Will this make one entry in the repr per row (and column) of a param table? Or am I misreading this?

You're misreading it. Take a look at the examples in the screenshots I've posted in this PR. Parameter Tables use their own built in html repr. #78 (comment)

However, it is quite messy to reconstruct the parameter tables from the _provider and _subprovider dicts, so it would be good if we could do that in a better way.

I thought we had decided to combine everything into a single table? I had thought those screenshots were thus outdated.

Okay I see. The parameters were merged with the providers in a single table. But the parameter tables are still displayed separately. Do you think it'd be better to merge them in the larger table as well? I think that it might be more messy. The parameter tables are self contained and have a html repr already so I thought it sensible to use that and display them separately.

Imho a user can display the param table themselves if they want to. The HTML repr of the pipeline should give info about the pipeline (i.e., mainly the structure), and as such I would consider each param table "column" as a single entity. That is, simply display a single summary line in the repr, instead of all the values. After all there might be thousands of rows in a param table? We must avoid making the pipeline's HTML repr unusable (or slow) in such cases.

Can we swap the "Source" and "Value" columns?

As I said before, I would be worried about size and lag when using this with large param tables. I think we should not include full nested HTML reprs of each value. Listing the size should be a good start.

It is generally always easier to make things more complicated later, than to remove it (because then some users who learned to love and rely on a feature will become upset).

I agree it might be a problem, but I think it is a unusual problem and one that is difficult to protect against in general. Displaying the html repr of any DataArray might lag if it is large enough. I also think being able to view the values of all parameters in the workflow like this is quite useful.

We talked before about displaying a reduced view of variables like is done in datagroup. The issue with that approach in this case is that we don't want the sciline code to contain lots of logic about how to display scipp datatypes.

I'm not sure about what you mean by listing the size, values of parameters can have any type and they don't have a common concept of size.

I'm not sure about what you mean by listing the size, values of parameters can have any type and they don't have a common concept of size.

I meant the number of rows of the param table.

So maybe were are talking about two different things now (because the discussion above switched back and forth)? I see I get this (no table of values):

Shouldn't the param-table info go into the source column, not the value column? Something like ParamTable(int, length=1000) maybe?

Yes it switched a bit back and forth and I understand it got confusing.

When we talked at my desk the other day we decided to only display the length of parameter tables. That's what's done now.

Now I thought we were talking about parameters that are not in tables. Just regular input values like QBins or Filename[Sample]. Those are currently displayed below a details tag in case they have a html repr. Otherwise they're just stringified.

Shouldn't the param-table info go into the source column, not the value column? Something like ParamTable(int, length=1000) maybe?

That sounds like a good idea. Maybe the source column should change name to "info" or something like that instead.

SimonHeybrock · 2024-01-10T10:28:19Z

Can you post some screenshots of the latest implementation?

Also, you have some merge conflicts to address.

jokasimr · 2024-01-10T10:37:42Z

Here are some screenshots:
One with collapsed fields and second with uncollapsed.
The value field is collapsed or not based on a cutoff for the string length.

SimonHeybrock · 2024-01-10T10:39:17Z

Can you also show one with a param table?

SimonHeybrock · 2024-01-10T10:41:46Z

tests/pipeline_test.py

+
+
+def test_html_repr() -> None:
+    pipeline = sl.Pipeline([make_int], params={float: 5.0})


Can we add generic providers and a param table for this (or separate) test, just to make sure this doesn't break the repr?

jokasimr · 2024-01-10T10:50:17Z

Here is a screenshot for a case with parameter tables:

SimonHeybrock reviewed Dec 4, 2023

View reviewed changes

jokasimr marked this pull request as ready for review December 14, 2023 13:20

jokasimr requested a review from nvaytet December 14, 2023 13:20

nvaytet reviewed Dec 15, 2023

View reviewed changes

YooSunYoung reviewed Dec 15, 2023

View reviewed changes

SimonHeybrock reviewed Dec 20, 2023

View reviewed changes

jokasimr force-pushed the html-repr-for-pipeline branch from ddc9873 to fe33e72 Compare January 3, 2024 09:54

jokasimr added 7 commits January 10, 2024 11:31

feat: add html repr for pipelines

679b30d

remove unnecessary header

3c4e9b7

fix: remove separate view for parameter tables

a6f1779

fix: some of the type errors

0d8437a

fix: change order of source and value

b946d4b

remove nested _repr_html_ of data arrays

5882651

display long parameter values below details tag

789390a

jokasimr added 3 commits January 10, 2024 11:31

fix: compatibility

96e296e

improve variable names

828e5e9

fix mypy

609456f

jokasimr force-pushed the html-repr-for-pipeline branch from 979978b to 609456f Compare January 10, 2024 10:31

Apply automatic formatting

73d6e45

SimonHeybrock reviewed Jan 10, 2024

View reviewed changes

add 'peek' view in detail

bfad72b

test: add test with parameter table

e25cbe8

jokasimr enabled auto-merge January 10, 2024 15:33

SimonHeybrock approved these changes Jan 11, 2024

View reviewed changes

jokasimr merged commit 5b462b6 into main Jan 11, 2024
5 checks passed

jokasimr deleted the html-repr-for-pipeline branch January 11, 2024 05:52



		def test_html_repr() -> None:
		pipeline = sl.Pipeline([make_int], params={float: 5.0})

Html repr for pipeline #78

Html repr for pipeline #78

Conversation

jokasimr commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokasimr commented Dec 8, 2023 • edited Loading

jokasimr commented Dec 8, 2023

jokasimr commented Dec 8, 2023

nvaytet commented Dec 11, 2023

SimonHeybrock commented Dec 11, 2023

nvaytet commented Dec 11, 2023

jokasimr commented Dec 11, 2023 • edited Loading

jokasimr commented Dec 11, 2023

jokasimr commented Dec 11, 2023

SimonHeybrock commented Dec 11, 2023

SimonHeybrock commented Dec 11, 2023

SimonHeybrock commented Dec 11, 2023

jokasimr commented Dec 11, 2023

jokasimr commented Dec 13, 2023 • edited Loading

SimonHeybrock commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokasimr Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokasimr Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

SimonHeybrock Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokasimr Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

SimonHeybrock Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

jokasimr Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

SimonHeybrock commented Jan 10, 2024

jokasimr commented Jan 10, 2024

SimonHeybrock commented Jan 10, 2024

Choose a reason for hiding this comment

jokasimr commented Jan 10, 2024

jokasimr commented Dec 8, 2023 •

edited

Loading

jokasimr commented Dec 11, 2023 •

edited

Loading

jokasimr commented Dec 13, 2023 •

edited

Loading

jokasimr Dec 19, 2023 •

edited

Loading

jokasimr Dec 20, 2023 •

edited

Loading

SimonHeybrock Dec 20, 2023 •

edited

Loading

SimonHeybrock Dec 20, 2023 •

edited

Loading

jokasimr Dec 20, 2023 •

edited

Loading

SimonHeybrock Dec 22, 2023 •

edited

Loading

jokasimr Dec 22, 2023 •

edited

Loading