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.

Sign up for GitHub

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

Jump to bottom

blocks-import-script: add --markdown flag and remove --csv-output flag #2985

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

blocks-import-script: add --markdown flag and remove --csv-output flag #2985

wants to merge 2 commits into from

Conversation

siddarthkay
Copy link
Contributor

@siddarthkay siddarthkay commented Jan 6, 2025 •
edited
Loading

Summary

This PR adds --markdown support to blocks import script.
This PR also removed --csv-output flag.

This option can be used like this

python scripts/block-import-stats.py
short-benchmark/20241208T172603_9207eaf/blocks-import-benchmark.csv
short-benchmark/20241222T012247_aba9b582/blocks-import-benchmark.csv 
--markdown

The output is a nice markdown table :

blocks-import-benchmark.csv vs blocks-import-benchmark.csv

Block Range BPS Baseline BPS Contender TPS Baseline TPS Contender Time Baseline Time Contender BPS Diff TPS Diff Time Diff
(20000001, 20111112] 25.62 24.98 4176.33 4075.37 1h9m29s 1h11m34s -2.63% -2.63% 2.84%
(20111112, 20222223] 25.91 26.95 4237.79 4408.99 1h13m53s 1h11m3s 4.01% 4.01% -3.84%
(20222223, 20333334] 26.60 27.69 4314.13 4490.81 1h7m0s 1h4m26s 4.05% 4.05% -3.87%
(20333334, 20444445] 29.13 30.55 4586.04 4811.19 1h5m47s 1h2m47s 4.83% 4.83% -4.59%
(20444445, 20555556] 29.12 29.79 4483.73 4588.42 1h0m59s 59m38s 2.29% 2.29% -2.22%
(20555556, 20666667] 29.62 30.58 4472.00 4619.38 1h4m33s 1h2m32s 3.26% 3.26% -3.14%
(20666667, 20777778] 30.50 31.44 4797.46 4947.33 58m15s 56m32s 3.08% 3.08% -2.97%
(20777778, 20888889] 30.99 32.87 4969.59 5273.27 1h1m50s 58m18s 6.08% 6.08% -5.72%
(20888889, 21000001] 30.81 32.98 5080.18 5438.98 1h2m16s 58m16s 7.00% 7.00% -6.53%

Summary

Metric Value
Total Blocks 991808
Baseline Time 9h44m5s
Contender Time 9h25m10s
Time Difference -18m55s
Time Difference % -3.24%

Legend

  • BPS Diff: Blocks per second difference (+)
  • TPS Diff: Transactions per second difference
  • Time Diff: Time to process difference (-)

(+) = more is better, (-) = less is better

@siddarthkay siddarthkay requested review from jakubgs and arnetheduck and removed request for jakubgs January 6, 2025 06:02
@yakimant
Copy link
Member

yakimant commented Jan 6, 2025 •
edited
Loading

(20000001, 20111112] - different brackets?

yakimant
yakimant requested changes Jan 6, 2025
View reviewed changes
scripts/block-import-stats.py Outdated Show resolved Hide resolved
scripts/block-import-stats.py
print("\nbpsd = blocks per sec diff (+), tpsd = txs per sec diff, timed = time to process diff (-)")
print("+ = more is better, - = less is better")
if args.markdown:
write_markdown_output(stats_df, df, args.baseline, args.contender)
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to treat markdown in the same way as CSV?
Currently it's CSV (optional) + MD or standard output. I would make it CSV (optional) + MD (optional) + standard output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently markdown is optional. If we don't pass --markdown flag only tab spaced data is returned in standard output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in latest commit I've made it more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

csv is also optional.
I think standard output is good for stdout, but for file formats better save them separately (csv, md,..)

scripts/block-import-stats.py Outdated Show resolved Hide resolved
@siddarthkay siddarthkay force-pushed the support-markdown-output-for-blocks-import branch from 505a445 to bb00e61 Compare January 6, 2025 12:08
@siddarthkay siddarthkay force-pushed the support-markdown-output-for-blocks-import branch from bb00e61 to 3a3eb26 Compare January 6, 2025 12:17
@siddarthkay
Copy link
Contributor Author

(20000001, 20111112] - different brackets?

In python : The brackets ( and ] in the output (20000001, 20111112] represent an "interval notation".

( means exclusive (does not include) the first number
] means inclusive (includes) the last number

@siddarthkay siddarthkay requested a review from yakimant January 6, 2025 12:26
@arnetheduck
Copy link
Member

my preference would broadly be that we cut down on the output formats to as few as possible - ie either csv or markdown - there's no point maintaining a bunch of formats that don't get used.

@siddarthkay
Copy link
Contributor Author

I like csv as an output format.
However judging by what you want as part of Readme on Github, the markdown table output seems like what we should keep as an output format.

@arnetheduck
Copy link
Member

whatever is minimally legible .. I'd even be fine with dumping the regular text output into triple-quotes or a table or a csv or any variation thereof. It doesn't greatly matter as long as it doesn't require multiple clicks to get to. The fewer the clicks and scrolling the better.

In terms of importance, I think the "main" readme that lists all benchmarks in a big table is the most important one - this is where we can get a quick overview whether a particular commit slowed something down. Having to drill down into each folder takes ages.

@jakubgs
Copy link
Member

jakubgs commented Jan 7, 2025

You can keep CSV and then just use a converter tool - one of many, like csv2md - and just convert it afterwards.

jakubgs
jakubgs approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

The markdown formatting code could use refactoring but it's good enough for now.

@siddarthkay siddarthkay changed the title blocks-import-script: add --markdown flag blocks-import-script: add --markdown flag and remove --csv-output flag Jan 7, 2025
yakimant
yakimant approved these changes Jan 7, 2025
View reviewed changes
scripts/block-import-stats.py
print("\nbpsd = blocks per sec diff (+), tpsd = txs per sec diff, timed = time to process diff (-)")
print("+ = more is better, - = less is better")
if args.markdown:
write_markdown_output(stats_df, df, args.baseline, args.contender)
Copy link
Member

Choose a reason for hiding this comment

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

csv is also optional.
I think standard output is good for stdout, but for file formats better save them separately (csv, md,..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubgs jakubgs jakubgs approved these changes

@yakimant yakimant yakimant approved these changes

@arnetheduck arnetheduck Awaiting requested review from arnetheduck

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@siddarthkay @yakimant @arnetheduck @jakubgs