-
Notifications
You must be signed in to change notification settings - Fork 287
chore(ci): add workflow to update documentation benchmark tables #2982
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.
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
base: main
Are you sure you want to change the base?
Conversation
691cce5 to
ff69a38
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me whether this actually works because of the svg updates
Nice to see it take shape though
@IceTDrinker reviewed 32 of 32 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @soonum)
.github/workflows/benchmark_documentation.yml line 68 at r1 (raw file):
op_flavor: default bench_type: both precisions_set: fast
no documentation set ?
.github/workflows/benchmark_documentation.yml line 86 at r1 (raw file):
uses: ./.github/workflows/benchmark_cpu_common.yml with: command: pbs, ks_pbs
do we have the ks benchmarks or was it removed ?
.github/workflows/benchmark_documentation.yml line 103 at r1 (raw file):
uses: ./.github/workflows/benchmark_gpu_common.yml with: profile: multi-h100-sxm5
what happens if we have an instance shortage ? I think it happened regularly in the past
.github/workflows/benchmark_documentation.yml line 105 at r1 (raw file):
profile: multi-h100-sxm5 hardware_name: n3-H100-SXM5x8 command: pbs, ks_pbs
same question
.github/workflows/benchmark_documentation.yml line 164 at r2 (raw file):
uses: actions/download-artifact@018cc2cf5baa6db3ef3c5f8a56943fffe632ef53 # v6.0.0 with: path: svg_tables
unclear to me that this works given each sub workflow will upload something under a different name ?
.github/workflows/benchmark_documentation.yml line 180 at r2 (raw file):
commit-message: | chore(docs): update benchmark results for all backends
whitespaces to remove
.github/workflows/generate_svg_common.yml line 50 at r2 (raw file):
run: | python3 -m pip install -r ci/data_extractor/requirements.txt python ci/data_extractor/src/data_extractor.py "${OUTPUT_FILENAME}" \
mix of python/python3 prefer python3 everywhere
.github/workflows/generate_svg_common.yml line 51 at r2 (raw file):
python3 -m pip install -r ci/data_extractor/requirements.txt python ci/data_extractor/src/data_extractor.py "${OUTPUT_FILENAME}" \ --generate-svg \
trailing whitespace to remove
.github/workflows/generate_svg_common.yml line 67 at r2 (raw file):
LAYER: ${{ inputs.layer }} PBS_KIND: ${{ inputs.pbs_kind }} GROUPING_FACTOR: ${{ inputs.grouping_factor || '4' }}
set the default in the inputs maybe ?
tfhe-benchmark/src/utilities.rs line 398 at r1 (raw file):
/// Get precisions values to benchmark. pub fn bit_sizes(&self) -> Vec<usize> { let bit_sizes_set = match self.bit_sizes_set {
maybe put the logic in the else block direcrtly ?
ci/data_extractor/src/data_extractor.py line 341 at r1 (raw file):
# In recent Python, dict keep insert order. # This call won't change metadata order in the suffix between runs. metadata_suffix += f"-{value}".lower()
not sure I understand what the change is about given it changes the suffix ?
.github/workflows/generate_svgs.yml line 122 at r2 (raw file):
# ----------------------------------------------------------- # PBs benchmarks tables
s capitalization does not match
.github/workflows/generate_svgs.yml line 160 at r2 (raw file):
# TODO: add table generation for backends comparison in 64 bits precision
backlog issue if you don't intend to do it now
.github/workflows/benchmark_gpu_4090.yml line 14 at r1 (raw file):
SLACK_USERNAME: ${{ secrets.BOT_USERNAME }} SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} BIT_SIZES_SET: FAST
env var is set twice ? (later in the step writing to github env)
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
svg uploads* not updates, I don't think the paths work as is
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @soonum)
ff69a38 to
fd9f98a
Compare
soonum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again we'll need to merge into main to be able to test it and take care of any edge cases.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker)
.github/workflows/benchmark_documentation.yml line 86 at r1 (raw file):
Previously, IceTDrinker wrote…
do we have the ks benchmarks or was it removed ?
We don't have the KS benchmarks in the documentation.
.github/workflows/benchmark_documentation.yml line 103 at r1 (raw file):
Previously, IceTDrinker wrote…
what happens if we have an instance shortage ? I think it happened regularly in the past
We could fallback on the permanent Hyperstack instance, this is handled in the sub-workflow.
That being said this instance is hibernated for the moment since we rarely used it.
So for now, this benchmark would fail.
As a mitigation strategy we could add extra checkbox [ ] Run CPU bench , [ ] Run GPU bench , [ ] Run HPU bench (all checkered by default). That way if one fail we could just relaunch the corresponding backend and still be able to perform the SVG generation afterward.
.github/workflows/benchmark_documentation.yml line 164 at r2 (raw file):
Previously, IceTDrinker wrote…
unclear to me that this works given each sub workflow will upload something under a different name ?
On the paper, it should work, considering the fact each artifact name is built around backend + layer + bench type + pbs-kind.
In our current usage we don't have collisions.
ci/data_extractor/src/data_extractor.py line 341 at r1 (raw file):
Previously, IceTDrinker wrote…
not sure I understand what the change is about given it changes the suffix ?
It's to warn devs about carefully do their insertion since it would change the final suffix.
But yes in it's current form, this suffix is changed to match documentation filenames formatting.
tfhe-benchmark/src/utilities.rs line 398 at r1 (raw file):
Previously, IceTDrinker wrote…
maybe put the logic in the else block direcrtly ?
Not really, in case of BitSizesSet::Fast and BitSizesSet::Documentation the vectors associated take precedence over the rest, hence the early return. I don't really see how I can improve that.
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IceTDrinker reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @soonum)
.github/workflows/benchmark_documentation.yml line 103 at r1 (raw file):
Previously, soonum (David Testé) wrote…
We could fallback on the permanent Hyperstack instance, this is handled in the sub-workflow.
That being said this instance is hibernated for the moment since we rarely used it.
So for now, this benchmark would fail.As a mitigation strategy we could add extra checkbox
[ ] Run CPU bench,[ ] Run GPU bench,[ ] Run HPU bench(all checkered by default). That way if one fail we could just relaunch the corresponding backend and still be able to perform the SVG generation afterward.
let's do the selection then
.github/workflows/benchmark_documentation.yml line 164 at r2 (raw file):
Previously, soonum (David Testé) wrote…
On the paper, it should work, considering the fact each artifact name is built around backend + layer + bench type + pbs-kind.
In our current usage we don't have collisions.
ok to be tested then
.github/workflows/generate_svgs.yml line 160 at r2 (raw file):
Previously, IceTDrinker wrote…
backlog issue if you don't intend to do it now
so was this TODO still relevant in the end ?
tfhe-benchmark/src/utilities.rs line 398 at r1 (raw file):
Previously, soonum (David Testé) wrote…
Not really, in case of
BitSizesSet::FastandBitSizesSet::Documentationthe vectors associated take precedence over the rest, hence the early return. I don't really see how I can improve that.
ah damn missed the early returns, all good then
This is done to ease automated table generation through continuous integration pipeline.
This is done to ease automated SVG tables for tfhe-rs public documentation.
2314b4c to
b80845d
Compare
This new workflow can trigger all the required benchmarks needed to populate benchmarks tables in documentation. It also can generate SVG tables and store them as artifacts. Optionally, it can open a pull-request to update the current tables in documentation.
b80845d to
38c63a3
Compare
This new workflow can trigger all the required benchmarks needed to populate benchmarks tables in documentation.
It also can generate SVG tables and store them as artifacts.
Optionally, it can open a pull-request to update the current tables in documentation.
This change is