Restore the Pausetimes Apps in the UI #130

punchagan · 2023-05-30T09:27:40Z

This PR re-enables the previously disabled Pausetimes apps. The apps have been updated to display the data in the new benchmark directories pausetimes_seq and pausetimes_par produced using olly latency.

The PR also:

Adds a script that fixes pausetimes bench files to be formatted similar to the new formatting implemented in Improve JSON output produced by olly gc-stats tarides/runtime_events_tools#13
Calls this script when copying data to the main branch to ensure any new data is formatted correctly. This is required until we are able to use the latest version of runtime_events_tools in Sandmark, which depends on ppxlib being compatible with OCaml 5.1 and 5.2.
Updates existing bench files pausetimes_* to use the new formatting, along with fixing an issue with the bench files having pretty printed JSON which breaks the assumption that every line in the bench file is a valid JSON string. This was fixed in Print the JSON for each pausetimes benchmark in a single line sandmark#452

kayceesrk · 2023-05-31T03:26:40Z

app/apps/instrumented_pausetimes_parallel.py

@@ -1,27 +1,21 @@
-import streamlit as st


The runtime is no longer instrumented. Best to call it pausetimes_parallel.py. Similarly for the sequential one.

kayceesrk · 2023-05-31T03:28:45Z

Left a minor comment. Otherwise, LGTM.

FWIW, I'm not able to review this UI/UX PRs easily without being able to test the app. We'll have to merge the PR and then fix any issues observed.

Do we have a staging area where we can deploy the app and then test it before we deploy it in production?

punchagan · 2023-05-31T03:57:20Z

@kayceesrk Thanks for the review!

Do we have a staging area where we can deploy the app and then test it before we deploy it in production?

We don't have a staging area, but I just deployed the branch on the Streamlit (free) hosting. You can check it out here.

The deployment doesn't seem to use the exact versions of dependencies specified in our requirements.txt, probably since they try to be smart by installing some dependencies (pandas, altair, etc.) by default for all the apps. Currently the Parallel pausetimes app is crashing, but the sequential one works. I'm looking around to see if there's a quick fix for the crash.

Left a minor comment. Otherwise, LGTM.

Thanks, I'll address this soon, along with any other comments you may have after using the UI.

punchagan · 2023-05-31T05:28:18Z

I'm looking around to see if there's a quick fix for the crash.

We were using an older version of streamlit in our requirements.txt and the deployment logic for supporting older versions of Streamlit on the cloud was causing issues with other dependencies. I've pushed a commit to update our dependency to the latest version of Streamlit, and everything seems to work okay.

@kayceesrk You can look at the deployed branch here for a more complete review. Thanks!

kayceesrk · 2023-06-02T03:57:36Z

The features look really awesome.

Remove "Instrumented" from all the locations.
Having both "sequential benchmarks" and "pausetimes sequential" seems odd. Can the "pausetimes sequential" appear at the bottom of the "sequential benchmarks" results?
In fact, the pausetimes are no longer computed on instrumented programs. If we are clever about isolating noise, the pausetimes numbers could be computed along with the sequential and parallel runs. FWIW earlier pausetimes were computed on the instrumented runtime, which caused slowdown of programs. This is no longer the case as the pausetimes are read from the shared ring buffer. This will cut down the running time of the nightly runs by 1/2 CC @Sudha247 thoughts?

Please do (1) in this PR as it is quick. For (2), given that the results are computed from different runs unlike suggested in (3), it might be a little tricky. As a stop-gap measure, may I suggest reordering the pages in the following way?

Goto
* Home
* Sequential - throughput
* Sequential - latency
* Parallel - throughput
* Parallel - latency

It may be useful to track (3) in a separate issue.

NOTE: The Perfstat output app uses a different `get_dataframe` implementation than the one used by the other apps which have their data in the JSON format.

We automatically copy successful outputs added by Sandmark builds from the testing branch to the main branch. Until Sandmark is updated to use the latest runtime_events_tools, the output would need to be fixed. This commit ensures that new bench files are fixed before being committed to the main branch.

Make the UI similar to other apps

We were using an outdated version of Streamlit 1.14.0 and this commit updates to the latest version 1.22.0. This makes it easier to deploy any PR branches of the app to Streamlit Cloud. With the older version, we run into a bug in the Streamlit deployment where the deploy process tries to be smart about "downgrading" Altair to a compatible version. I'm not sure why a different version of Altair is installed in the first place, and why it is later downgraded. This results in different versions of some packages being installed than those specified in our requirements.txt.

punchagan · 2023-06-02T13:11:40Z

@kayceesrk Thanks for the review and the suggestions to make the results less confusing. I've changed the UI and the names of the apps as suggested by you. The staging deployment from this branch can be viewed here.

Sudha247 · 2023-06-02T13:43:08Z

I also think we can extract both running times and pausetimes from the same run. I've observed the overheads added by olly to be quite low, though we may have to confirm this for the benchmarks in sandmark. Good idea to track this in a separate issue.

We use a unique slug for apps that is used in the URL query parameters to allow changing the app titles without breaking old/existing URLs. This change also ensures that any existing production app URLs in the wild are still functional.

punchagan · 2023-06-02T14:38:30Z

Good idea to track this in a separate issue.

I've created ocaml-bench/sandmark#454. Feel free to edit or add more information.

kayceesrk · 2023-06-03T03:43:45Z

LGTM. Thanks.

punchagan requested review from shakthimaan and kayceesrk May 30, 2023 09:27

kayceesrk reviewed May 31, 2023

View reviewed changes

kayceesrk approved these changes May 31, 2023

View reviewed changes

punchagan force-pushed the restore-pausetimes branch 2 times, most recently from a77824c to 4331547 Compare June 2, 2023 07:17

punchagan added 14 commits June 2, 2023 13:47

Extract get_dataframe as a utility function from all the apps

30774af

NOTE: The Perfstat output app uses a different `get_dataframe` implementation than the one used by the other apps which have their data in the JSON format.

Add a script to fix JSON format issues with pausetimes bench files

1e31877

Fix the pausetimes bench files using the cleanup script

047abea

Enable the sequential Instrumented Pausetimes app

4cac49c

Enable the parallel Instrumented Pausetimes app

c917c95

Display selected bench file metadata expanded

e0839bc

Make the UI similar to other apps

Remove sorting of selected bench files as with other apps

fea162e

Save UI state of pausetimes apps as URL query parameters

a109b94

Move app title, function and saved session keys to a single config

bd15a14

Minor imports fixes

6dbfd64

Minor formatting fixes

36c9dec

Remove Instrumented since the benchmarks are no longer instrumented

93c76b6

punchagan force-pushed the restore-pausetimes branch from b75454e to 6f82f00 Compare June 2, 2023 12:44

punchagan added 3 commits June 2, 2023 14:48

Add a README for explaining the test setup and running the tests

ff4a0b8

Extract code for copying artifacts for tests

cbe50dd

Add UI tests to verify no tracebacks in the latency pages

c5a4c75

punchagan force-pushed the restore-pausetimes branch from 6f82f00 to c5a4c75 Compare June 2, 2023 12:55

punchagan mentioned this pull request Jun 2, 2023

Get latency and throughput information from the same benchmark runs ocaml-bench/sandmark#454

Open

kayceesrk merged commit 1e29b73 into ocaml-bench:main Jun 3, 2023

punchagan mentioned this pull request Jun 3, 2023

Add latency graphs in sequential/parallel web pages #114

Closed

punchagan deleted the restore-pausetimes branch June 5, 2023 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore the Pausetimes Apps in the UI #130

Restore the Pausetimes Apps in the UI #130

punchagan commented May 30, 2023 •

edited

Loading

kayceesrk May 31, 2023

kayceesrk commented May 31, 2023

punchagan commented May 31, 2023

punchagan commented May 31, 2023

kayceesrk commented Jun 2, 2023 •

edited

Loading

punchagan commented Jun 2, 2023

Sudha247 commented Jun 2, 2023

punchagan commented Jun 2, 2023

kayceesrk commented Jun 3, 2023

Restore the Pausetimes Apps in the UI #130

Restore the Pausetimes Apps in the UI #130

Conversation

punchagan commented May 30, 2023 • edited Loading

kayceesrk May 31, 2023

Choose a reason for hiding this comment

kayceesrk commented May 31, 2023

punchagan commented May 31, 2023

punchagan commented May 31, 2023

kayceesrk commented Jun 2, 2023 • edited Loading

punchagan commented Jun 2, 2023

Sudha247 commented Jun 2, 2023

punchagan commented Jun 2, 2023

kayceesrk commented Jun 3, 2023

punchagan commented May 30, 2023 •

edited

Loading

kayceesrk commented Jun 2, 2023 •

edited

Loading