darshan-hpc / darshan

Darshan I/O characterization tool
Other
57 stars 28 forks source link

ENH: reorganize PyDarshan summary reports and add an overview table #927

Closed shanedsnyder closed 1 year ago

shanedsnyder commented 1 year ago

I tried to accomplish a couple of goals here:

  1. Force specific module figures to always appear in the same location for each module.
  2. Add a simple overview table for modules that support accumulation that replaces the performance estimate string.

Without 1), our current CSS grid for module data just packs 2 figures to a row without any real mechanisms to control how many figures can go on a line, which figures can be positioned side-by-side, etc. Figures are currently added in the order they are registered in the summary code, but different modules have different number of figures so there's not really a simple way to ensure figure organization is uniform across modules.

To help with this, I quickly adopted some CSS code to more explicitly place module figures in a fixed grid. The summary code can now specify a class for registered report figures which corresponds to grid areas maintained in the CSS stylesheet. This allows us to ensure figures are placed in certain locations, that some figures are not meant to share lines, etc. We will need to take care to maintain this as we add new figures -- if figures don't specify an explicit grid area they are placed in the first available grid area, which might throw things off. New figures are added seldom enough that I doubt this is a huge deal.

For 2), I've just added a new overview table that highlights a few key details about modules that support accumulation. This includes the performance estimate, which still is displayed in blue text (though this is accomplished via CSS now) -- pandas style seems to require Jinja and I'd like to avoid bringing it in if we can help it.

A couple of smaller things I've done:

TODO before merge:

shanedsnyder commented 1 year ago

Here's some example output showing how figure placement is being enforced.

The POSIX module forces the operation counts and access pattern plots next to each other:

image

As seen above for POSIX and below for MPI-IO, access size histograms and common access tables are always side-by-side:

image

shanedsnyder commented 1 year ago

And here's an example of the new overview table which comes first for modules that support it: image

shanedsnyder commented 1 year ago

It might be easier to keep CSS/layout changes separate from other code changes, in another PR.

The main reason I included all in one PR is just that some of the changes started becoming a little intertwined. E.g., I couldn't find a way to make the prominent blue text in the module overview table without the CSS changes. I can try to revise the PR if we think the CSS changes are a bad idea, so it's not like the changes have to be all or nothing. Mainly wanted to bring this all up for discussion, which is also why I didn't really spend anytime worrying about testing yet. A WIP label probably would have been the right call in hindsight.

I think I'm probably -0.5 on the CSS changes vs. just controlling the insertion order and getting 80 % of the way there.

I can obviously concede what we have now is easier. I do still think there's value in the CSS changes though, even if this value may arguably not be realized straight away:

The changes are also relatively small (1 new arg to ReportFigure, a small change in the base HTML code, and 10-15 lines in the CSS). Adding a new explicit report region just requires adding a couple of lines of CSS and using the new arg (fig_class) to ReportFigure. Further, these changes are optional -- if we were to add new figures to the report and forgot to (or didn't care to) use this new CSS class logic, the end result is that the figures are just placed in the default grid in insertion order (well, placed after any explicit regions that this PR introduces).

As for the flow of the other plots, that seems pretty subjective based on what someone is looking for anyway, so a strict adherence to a particular layout doesn't seem worth the CSS stuff to me at the moment

Well sure, most of the decisions for how to organize or display this stuff visually will be subjective to some degree, but that doesn't mean we can't take our best shot at organizing things in a way we think is logical. I'll admit the need to place things one per-row or forcing figures side-by-side isn't immensely critical right now, but it seems to me that inevitably if we want to have more control or structure over organization we're going to have to tinker with CSS at some point. For example, if we wanted to use a text box warning beside a specific figure, we have no way to control that now. I suspect it's only a matter of time until we have more examples of things we'd like to organize better.

TL;DR: I think the CSS stuff could provide value, even if currently it's relatively low impact. I'd hate to throw out code that basically works and gives us more control at relatively low maintenance cost just to have to revisit in the future when we have other examples of things that CSS could help with.

tylerjereddy commented 1 year ago

I'd hate to throw out code that basically works and gives us more control at relatively low maintenance cost

I don't think you need to throw code out, git branching provides a pretty clear path forward for preservation of code if we aren't ready to commit to the CSS stuff yet. That said, I don't really object that strongly, but I will ask that tests to enforce the ordering are well written and maintainable/easy to adjust if the order changes in the future, if we are going to commit to enforcing an order.

tylerjereddy commented 1 year ago

The main reason I included all in one PR is just that some of the changes started becoming a little intertwined. E.g., I couldn't find a way to make the prominent blue text in the module overview table without the CSS changes.

pandas provides a mechanism for coloring text in tables that uses CSS under the hood, without actually imposing CSS on Python developers: https://pandas.pydata.org/docs/user_guide/style.html. Shielding from using Jinja seems like something to discuss vs. hand-usage of CSS tradeoffs..

shanedsnyder commented 1 year ago

In recent commits I:

I started to work on tests to enforce the figure order on the output HTML report string, but I'm actually not sure how we would do that. The ordering is enforced by the CSS, which is just copied in statically to the top of the HTML code. The ordering of the different HTML sections in the code is still in insertion order from the summary script, so checking that order doesn't seem useful. I don't see any other tests for other aspects of our CSS (e.g., "Job summary" table, "Darshan log information" table, header, footer), so I'm not sure it's really a big deal.

I don't have any additional planned changes here at the moment. I'm not sure there's another way (e.g., using pandas style) to enable this sort of control on how different report figures are located or styled without getting into the CSS code ultimately. And mentioned above, but we're already using CSS to enforce different organizational/aesthetic things anyways, this is just extending our usage of it into the per-module report sections.

tylerjereddy commented 1 year ago

The ordering is enforced by the CSS, which is just copied in statically to the top of the HTML code. The ordering of the different HTML sections in the code is still in insertion order from the summary script, so checking that order doesn't seem useful.

It seems problematic that we can't easily test the order, I'm not a huge fan of that to say the least, and it is just a recipe for allowing things to slip under the radar. I think it is still possible with great effort, but you need to do a browser-based image analysis testing, a bit like mocking what a human would see. This wouldn't happen if we used Python insertion order into the HTML to control the layout.

I don't see any other tests for other aspects of our CSS

I don't think existing gaps in our test suite are a good reason not to write quality tests moving forward, that seems dangerous.

Ultimately, it sounds like you really want to do this, so there's not much I can do, but I also note that I don't really see much convincing user or even developer feedback asking for this at this time.

carns commented 1 year ago

This seems like a pretty simple but very practically useful set of changes to me.

The side-by-side control is actually a pretty big deal. It's frustrating from a user experience point of view if two figures that are intuitive to compare aren't laid out conveniently to do so. We need to streamline what we can so that people can focus on the information they are interested in quickly without spending too much time orienting themselves in the report. I like the at-a-glance summary table being the first thing for each module as well for similar reasons.

I guess in summary it looks like the benefit is worth the cost, for my 2c.

It's not intuitive to me that figure ordering lends itself well to CI testing regardless of the method, particularly if you are attempting to group things. Whether things are technically in the order you specified is secondary compared to if that order produced a sensible and helpful end result visually. Layout is hard. That's will probably remain a human test rather than an automated test for now.

tylerjereddy commented 1 year ago

It's not intuitive to me that figure ordering lends itself well to CI testing regardless of the method

I don't think the runtime testing was particularly easy to get going either, but I put in the time to make that work. At a minimum, a quick check of available CSS unit testing frameworks like https://github.com/jamesshore/quixote/blob/master/README.md could be done.

Quixote is unique for its ability to test how elements relate to each other. For example, you can test if one element is below another element, or how an element compares to the browser's viewport.

I'll try to take another look at this today then, since Argonne seems to want it merged, but I do think that we shouldn't be dismissive of testing things just because we don't know how at first glance, and some caution about the complexities of CSS interactions seems warranted, especially given the increased reliance on human visual inspection that's proposed and the low likelihood that we get CSS expert reviews.

tylerjereddy commented 1 year ago

I've merged it, but I will open a separate issue about the testing matter.

carns commented 1 year ago

It's not intuitive to me that figure ordering lends itself well to CI testing regardless of the method

I don't think the runtime testing was particularly easy to get going either, but I put in the time to make that work. At a minimum, a quick check of available CSS unit testing frameworks like https://github.com/jamesshore/quixote/blob/master/README.md could be done.

Quixote is unique for its ability to test how elements relate to each other. For example, you can test if one element is below another element, or how an element compares to the browser's viewport.

I'll try to take another look at this today then, since Argonne seems to want it merged, but I do think that we shouldn't be dismissive of testing things just because we don't know how at first glance, and some caution about the complexities of CSS interactions seems warranted, especially given the increased reliance on human visual inspection that's proposed and the low likelihood that we get CSS expert reviews.

Fair enough; my comment arises from the fact that I don't know what I don't know :) If there are reasonable tools available for this purpose then I have no objection to using them.