Skip to content

Output HTML and JSON from codegen diff tool#996

Merged
jacobhinkle merged 41 commits intomainfrom
html_codegen_diff
Oct 4, 2023
Merged

Output HTML and JSON from codegen diff tool#996
jacobhinkle merged 41 commits intomainfrom
html_codegen_diff

Conversation

@jacobhinkle
Copy link
Collaborator

@jacobhinkle jacobhinkle commented Sep 29, 2023

I have been chasing down codegen changes in #840 and #947 and have needed to dig through a lot of spurious diffs. I decided to extend the codegen diff tool to output HTML, and to also modify the diffing a bit. This PR:

  • Changes tools/compare_codegen.sh to output env information as well as add ptxas_verbose dump option.
  • Changes the diffs performed by that tool to ignore both the kernel name and the preamble. The preamble is estimated by skipping the typedef of nvfuser_index_t. If preambles between two runs differ, we report that with a warning and show the diff in the output.
  • Adds an --html option to tools/diff_codegen_nvfuser_tests.py which will write a self-contained HTML file holding all the differing kernels and diffs. To use this option you must have previously run pip install jinja2.
  • Adds a --json option to tools/diff_codegen_nvfuser_tests.py which writes a JSON file containing all the information contained in the HTML file in an easier-to-parse format.
  • Changes the default to not printing the diffs to STDOUT. This can be re-enabled with the --show-diffs argument.

This lets us communicate code differences easily by sharing these files, which could be generated by our CI. An example output is attached.

Github doesn't support uploading html so I have uploaded a zipped example:
codediff_f7786819_feda1e1e_binary_tests.html.zip

Note that this file is probably typical for a medium sized change: it results in a zipped file size of 184KB and unzipped it is 2.1MB.

Some ideas left out of this PR that might be nice in the future:

  • Handle not just nvfuser_tests output but also nvfuser_bench and pytest output. We could also fall back to arbitrary command output where we just group everything to one big "test" if we can't associate each kernel with a specific test/benchmark.
  • Show multiple commands in one HTML file. Especially if the first bullet is addressed, then we could have a single summary for our whole suite.
  • Include benchmark results. This could be done in another hidden div with a "benchmarks" button. It might be tricky especially if the number of benchmark items associated to each kernel is changed between commits, but it might also be handy to refer to benchmark regressions and have the codegen output one click away.

Fixes #1007

@xwang233
Copy link
Collaborator

Thanks for adding the HTML output. That looks great!

Does the python script tools/diff_codegen_nvfuser_tests.py compare kernels based on test name but not file name on all test suites? I recall that nvfuser_tests works now but not other test suites like python tests or nvfuser_bench.

Nvfuser_bench is different from all other test suites (nvfuser_tests, python tests) because it prints test name (benchmark name) after kernel dump file (PRINTING: __tmp_kernel ...) while all other test suites print test name before the kernel dump file.

@jacobhinkle
Copy link
Collaborator Author

Does the python script tools/diff_codegen_nvfuser_tests.py compare kernels based on test name but not file name on all test suites?

Yes it compares based on test name and it's specific to nvfuser_tests. We could update it pretty easily to also handle pytest since that prints the test name before the kernels

Nvfuser_bench is different from all other test suites (nvfuser_tests, python tests) because it prints test name (benchmark name) after kernel dump file (PRINTING: __tmp_kernel ...) while all other test suites print test name before the kernel dump file.

We could probably enable this too if we're clever, we just need to accept some regexes and plumb them around.

@jacobhinkle jacobhinkle marked this pull request as ready for review September 30, 2023 00:30
@xwang233
Copy link
Collaborator

xwang233 commented Oct 1, 2023

Can we add an option to only dump limited number (say 200) of kernel comparisons? We can add a prompt at the end of that page saying something like, e.g. "Only dumped 200 of 10086 total mismatches. To dump all the mismatches, please do xxx".

This is to make the generated file not being too huge in size.

@jacobhinkle
Copy link
Collaborator Author

Can we add an option to only dump limited number (say 200) of kernel comparisons? We can add a prompt at the end of that page saying something like, e.g. "Only dumped 200 of 10086 total mismatches. To dump all the mismatches, please do xxx".

This is to make the generated file not being too huge in size.

Good idea. There are a few other things I'd like to add before merging too so I'll add this to the list.

@jacobhinkle
Copy link
Collaborator Author

There are a few other things I'd like to add before merging

Btw one of those is an option to exclude the preamble which can decrease file size a bit. Still if a change impacts tons of tests we could easily wind up with hundreds of diffs which is probably not ideal for a ci artifact. At least they seem to compress well..

I will use this but not show it directly. Instead, I'll parse it and
show the info on each kernel line, along with possible index type change
and number of lines added/removed.
This reduces file size considerably. The original 11MB uncompressed file is now
2.0MB.
Copy link
Collaborator

@xwang233 xwang233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated webpage looks really cool! Thanks for adding this.

@jacobhinkle jacobhinkle changed the title Output html from codegen diff tool Output HTML and JSON from codegen diff tool Oct 3, 2023
@jacobhinkle
Copy link
Collaborator Author

@xwang233 I am not sure what impact this will have on CI when merged. Note that we no longer print diffs to screen by default. We can do that if needed with --show-diffs, but I also included a --json <filename> argument that gives us a structured version instead. The exit code is not the number of diffs (which can easily be over 256) but is 1 if there are differences in either the preamble or kernels and zero if otherwise.

@jacobhinkle
Copy link
Collaborator Author

!build

@jacobhinkle
Copy link
Collaborator Author

!build

@xwang233
Copy link
Collaborator

xwang233 commented Oct 3, 2023

Should the python script be called after the bash script? The python script hasn't been integrated into the CI yet. I'll check those later today.

@jacobhinkle
Copy link
Collaborator Author

Should the python script be called after the bash script? The python script hasn't been integrated into the CI yet. I'll check those later today.

Yes, tools/compare_codegen.sh will create a directory called by default codegen_comparison/. Under that will be two directories, named after the 8-character hash of the git commits on main and the PR branch. Those two directories are the required args to python tools/diff_codegen_nvfuser_tests.py. Until we update CI to upload an HTML report or to parse the JSON, we should probably add the --show-diffs so that this tool will behave the same as before. In fact, I think no changes to CI will be needed right away if I just change it to show the diffs by default and replace that arg with a --hide-diffs arg. I'll give that a go. Note that the codediff command seems to have worked in the previous CI run though; it's nvfuser-ci that says it fails (reference B#8206-J#70698158).

The --show-diffs arg actually had no effect (oops). Fixed that also.
@jacobhinkle
Copy link
Collaborator Author

!build

@xwang233
Copy link
Collaborator

xwang233 commented Oct 3, 2023

Thanks for that. The failure in nvfuser-ci with reference numbers doesn't necessarily mean it's the codegen-diff job failure. It could be something else flaky in network. Don't worry about that.

@jacobhinkle
Copy link
Collaborator Author

Ah OK. If CI fails again I'll just merge without a !build then.

@jacobhinkle jacobhinkle merged commit f24e61f into main Oct 4, 2023
@jacobhinkle jacobhinkle deleted the html_codegen_diff branch October 4, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should the codegen diff tool only check the kernel itself?

2 participants