Repository navigation
examples: add a Dockerised engine benchmark covering every supported Python version - #410
Merged
Merged
Conversation
examples/benchmark/run.sh # or: make bench The engine speed table in the README could not be reproduced or even completed. No single machine can measure every engine: `pypcap` and `pcap_ct` both own the import name `pcap` so they are mutually exclusive; `pypcap` needs a compiler plus libpcap headers and won't build past 3.11; `pyshark` needs the `tshark` binary and can't run on 3.14; `pypcapfile` can't be imported on 3.12+. Python 3.11 is the last interpreter where all seven run, and even there two of them can't coexist. So the harness runs **two virtualenvs inside one pinned image** -- one with `pypcap`, one with `pcap_ct`, everything else in both -- and stitches the two runs together through the `default` engine, which is present in both. Verified the join holds: `dpkt` measures 0.0617 in one venv and 0.0614 in the other. It reports **ratios to `default`, not absolute times**, because absolute ms/packet is only meaningful on the machine that produced it -- this host disagrees with the README's existing figures in *both* directions. One absolute baseline is kept as a footnote so the ratios can be converted back. It also reports the observed range per engine and states the noise floor, so a reader knows which gaps are real: the median row varied 1.3% and the widest 2.7% across passes, so anything smaller is noise. Every measurement asserts the engine that actually ran. A missing or unusable engine only *warns* and silently falls back to pcapkit's own parser, so a timing run that appears to succeed can be timing the wrong thing entirely; an escalated `EngineWarning` or a zero-packet capture ends the run rather than publishing a false number. A real failure mode found by the first full run, and worth the two levels of handling it now has: `tshark` crashed on pass 3 after ~2,000 spawns (`TSharkCrashException`, retcode 255), the exception propagated, and the run produced **no output at all** -- six working engines lost to the seventh hiccupping once. A failed extraction is now discarded and counted, a failed pass costs that engine only that pass and is marked in the table, and an engine becomes `*not measured*` only if every pass failed. `run.sh` also used to claim it wrote the table after that crash, because `docker cp` of an empty directory succeeds; it now checks what actually landed. No `--platform` override is needed on Apple Silicon: the base image is a multi-arch index including `linux/arm64/v8`, every engine package is either arch-independent or has aarch64 wheels, and `pypcap` -- the only compile -- has no arch-specific code. Built and ran all seven engines on `linux/arm64` under QEMU to confirm availability, then removed the emulation. Native arm64 *timings* are unverified and are not claimed; if `--platform linux/amd64` is forced, `run.sh` warns on stderr **and** threads the caveat into the emitted table, since whoever reads the table later is not whoever saw the stderr. The emitted snippet is plain-docutils-safe -- no Sphinx-only roles -- and carries the image digest, pcapkit revision, Python version, architecture, capture and its SHA-256, iteration and pass counts, and per-venv package versions. Nothing host-identifying: the point of containerising is that the host stops mattering. Verified: 55 self-tests pass, `shellcheck` clean, project suite unaffected at 644 passed / 8 skipped, emitted RST parses under plain docutils with zero messages, and ratios reproduce across independent runs (`dpkt` 0.0616/0.0621, `scapy` 0.151/0.153, `pyshark` 89.1/89.1). Two results contradict the current README and want a look before anything is pasted: `pypcapfile` measures *faster* than `dpkt` (0.0548 vs 0.0616), reversing the published ordering, and `pyshark` is 89x rather than the 123x implied today.
There was a problem hiding this comment.
🟡 Changes recommended
The report stitching logic in examples/benchmark/report.py uses truthiness checks for baseline divisors and records environments even when no ratios were contributed, which can lead to incorrect cross-environment reporting.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a containerised benchmarking harness under examples/benchmark/ to reproducibly generate the engine speed table (as paste-ready RST) from a pinned Docker image, including self-tests and Makefile entry points to run the suite with one command.
Changes:
- Add a Docker-based benchmark runner (
run.sh+Dockerfile+entrypoint.sh) that measures engines across two isolated virtualenvs and emits a README-ready RST snippet plus raw JSON/locks. - Add the benchmark measurement and reporting implementation (
benchmark.py,report.py) and a dedicated self-test suite (test_harness.py). - Add pinned requirements files for the two environments and documentation (
examples/benchmark/README.md) plus Make targets (bench,bench-quick,bench-test).
File summaries
| File | Description |
|---|---|
| Makefile | Adds bench, bench-quick, and bench-test targets to run the harness and its self-tests. |
| examples/benchmark/benchmark.py | Implements per-environment engine timing, driver assertion, and JSON output contract. |
| examples/benchmark/report.py | Stitches per-env JSON documents and renders operator text + README-ready RST tables. |
| examples/benchmark/test_harness.py | Adds self-tests for ratio math, stitching, markup validity, and failure handling. |
| examples/benchmark/run.sh | Host-side script to build/run the image and copy out/render results. |
| examples/benchmark/entrypoint.sh | Image entrypoint to run measurements in both venvs then generate reports. |
| examples/benchmark/Dockerfile | Builds a pinned-image benchmark environment with two venvs and required system deps. |
| examples/benchmark/README.md | Documents the benchmark’s rationale, methodology, and how to run it. |
| examples/benchmark/requirements-common.txt | Pins shared engine/runtime dependencies for reproducible runs. |
| examples/benchmark/requirements-pypcap.txt | Pins the pypcap-specific environment requirements. |
| examples/benchmark/requirements-pcap_ct.txt | Pins the pcap-ct-specific environment requirements. |
Review details
Suppressed comments (1)
examples/benchmark/report.py:708
- In
_ratios_for(),if by_repeat.get(sample['repeat'])is a truthiness check that will also drop repeats where the baseline divisor is 0, and it duplicates the baseline-handling logic fromcollect()in a less explicit way. Switching to an explicitNone/zero check makes the behavior clearer and avoids surprises from falsy numeric values.
by_repeat = {sample['repeat']: sample['ms_per_packet'] for sample in base['repeats']}
return [sample['ms_per_packet'] / by_repeat[sample['repeat']]
for sample in entry['repeats'] if by_repeat.get(sample['repeat'])]
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses two review findings on #410, both in the normalisation step: - ``if not divisor`` treated a baseline of ``0.0`` as a missing baseline. Both cases still drop the sample -- neither can be divided by -- but they are different problems, so the rule moves into ``_usable_baseline()`` which says which is which, and the second copy of it in ``_ratios_for()`` now shares it. - An environment was recorded whenever the engine had any repeats, even when every one of them lost its baseline and contributed no ratio. That put the engine into the cross-environment check, which then printed a one-sided "the two should agree" line. It is now keyed on a ratio having survived. Two self-tests added; the environment one fails against the previous code. 57 pass.
The suite built one image (3.11) and could report a ratio table only. It now builds one image per interpreter and emits the complete per-version table the project README maintains by hand. * `python-images.txt` is the matrix: base image per version, pinned by digest, plus whether that interpreter can hold `pypcap` and whether a plain run covers it. A table rather than logic, so a reader can check the digests were chosen. * Dockerfile takes `PYTHON_IMAGE` and `WITH_PYPCAP`; on 3.12+ it skips the `pypcap` virtualenv, which cannot be filled there, and records the interpreter ceiling so the report says the engine is unsupported rather than broken. The environment labels are derived from the interpreter itself, so a build cannot put one version's figures in another's column. * `run.sh` loops the versions, gains `--pythons`, and contains a per-version build or run failure as a `--` column with the reason attached instead of aborting the matrix. Still bash 3.2, still one command with no arguments. * `entrypoint.sh` gains a reporting mode, since no single interpreter measured the whole matrix; it runs in one of the images so docker stays the only host requirement. * `report.py` emits `table-versions.rst` -- absolute ms/packet, engines by Python version, paste-ready for README.rst. Absolute deliberately: ratios normalise within an environment, so one taken across two interpreters describes neither. The ratio table keeps working and is now headed `Test Results (Relative)`. Verified with a six-version quick matrix (3.10-3.15) and with runs that force a missing column; both emitted tables parse under plain docutils. Self-tests 98 passed, up from 57.
) Review findings on the matrix change. The first is a defect in the tables the suite promises can be pasted into README.rst verbatim. * Reasons are written by compilers and exceptions, not by this harness, and were interpolated into RST as prose. Measured under plain docutils: `**kwargs` in a gcc line, GNU's `cannot find `pcap.h` quoting, a trailing `imp_`, and anything ending `::` each turn the snippet into a visible error block. Reachable by the documented path, since the Dockerfile records the tail of a failed pypcap build as that engine's reason. Escaped at every site where outside text enters markup. * `render_text` called a partly measured version "not measured" while printing its figures three lines above. The per-version failure lines are gone from that block; the provenance rows above already say it, once, correctly. * Guarded the two `docker image inspect` substitutions in the version loop -- under `set -e` a failure there aborted the matrix, which is what the loop exists to prevent -- and deduplicated `--pythons`, whose repeats collided on container name and were then reported as spurious gaps. * `--engines` without `default` now fails before building rather than as a traceback from the reporting container an hour later. * A run that lost a column exits 3: the tables are written, so it is not a failure, but it should not be indistinguishable from success either. * Renamed the ratio table's `Passes` column to `Samples`, since a pass happens once per environment and the two numbers differ by a factor of seven on a full matrix. * A packet-count disagreement between interpreters is now reported instead of silently dropping the row -- it would be the most important thing in the report. * `README.md` and the Dockerfile called 3.11 the only interpreter running every engine; 3.10 does too. It is the last one, which is what README.rst says. Adds `TestPinsFile`, which validates the matrix file `run.sh` parses with awk and catches the Dockerfile's duplicate 3.11 digest drifting, and `TestHostileReasons`, which drives both renderers with strings a compiler really produces. 115 passed.
…nel (#410) `--emulated` was the one path by which outside text still reached the emitted markup unescaped, in two places: a provenance row and a bold paragraph in each snippet. It is safe in practice because `run.sh` composes that sentence itself, which is precisely why it was the one left over. Escaped at each renderer's own boundary rather than inside `_provenance`, which also feeds the plain-text report and would otherwise show the operator backslashes -- the same split `_missing_pairs` already makes, for the same reason. The sentence `run.sh` actually passes contains no markup characters, so its rendered output is unchanged. Test drives both renderers with an emulation note carrying `**` and a stray backtick. 116 passed.
The project README is `.rst`, so this one being `.md` was the odd one out. `git mv` keeps the history. Markdown tables become `list-table` directives -- the cells carry sentences, and a simple table would need column widths wider than the text. The ` ` paragraph-indent entities are gone: they are a Markdown-only hack and would show as literal text in RST. RST does not support nested inline markup, which the straight conversion tripped over in seventeen places -- ``**``pypcap`` is exclusive**`` leaves the backticks visible rather than rendering as code. Each one is now either bold or literal, not both, chosen per case: bare names where the sentence reads fine bolded, and literal-only for the troubleshooting bullets, whose subject is an error string. Verified through the parsed doctree rather than by eye, since docutils does not warn about the nesting -- it silently keeps the backticks as text. No system messages, no problematic nodes, no strong or emphasis node containing a backtick, 16 sections and 4 tables. The one `:mod:` in the file is inside double backticks, where it is prose about the role rather than a use of it. Also points `benchmark.py`'s docstring reference at the new filename.
JarryShaw
added a commit
that referenced
this pull request
Sep 16, 2026
Second of the two, after the benchmark suite's on #410. The project README is `.rst`, so nothing in the repository is Markdown any more. `git mv` keeps the history. The script table becomes a `list-table`, and each script name stays a link to its own file -- `` `test_basic <test_basic.py>`_ `` renders on GitHub the same way the Markdown link did. The ` ` paragraph-indent entities are dropped, being a Markdown-only hack that RST would show as literal text. Where bold wrapped a code span, one of the two had to go: RST does not nest inline markup, and `**`code`**` leaves the backticks visible instead of rendering. The engine-list bullets keep bold on the bare script names; `TZ=UTC` keeps the literal and moves the bold onto the sentence around it. Verified through the parsed doctree rather than by eye, because docutils does not warn about that nesting -- it silently keeps the backticks as text: no system messages, no problematic nodes, no strong or emphasis node containing a backtick, and all 19 script links resolve. No tracked file references the old name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One invocation builds an image per interpreter (3.10 through 3.14 by default, 3.15 opt-in), measures seven environments, and writes two paste-ready reStructuredText snippets. A full default matrix is on the order of two hours, most of it
pysharkspawning atsharkper extraction;--engines default,dpkt,scapy,pypcap,pcap_ct,pypcapfiledrops it to minutes, and--pythons 3.11to one column.Why this exists
The engine speed table in the README can't be reproduced, and can't even be completed on one machine:
pypcapandpcap_ctare mutually exclusive — both distributions own the import namepcap, and with both installed pcap-ct wins while upstream is shadowed.pypcapneeds a compiler plus libpcap headers, and won't build past 3.11.pysharkneeds thetsharkbinary and can't run on 3.14.pypcapfilecan't be imported on 3.12+.Python 3.11 is the last interpreter where all seven run, and even there two can't coexist. That used to be a constraint on the run; it is now a fact the table reports, as
--cells carrying each engine's own reason.The matrix
One image per Python version, each pinned by digest in
python-images.txt— a table rather than logic, so a reader can check it, and adding a version is one line. Inside each image, one or two virtualenvs, so an environment is a (Python version, pcap provider) pair:-pypcapand-pcap_ct-pcap_ctonlypypcapcan't be installed, so a second venv would compile for minutes to produce nothingEnvironment labels are derived by asking each interpreter its own version, so a mislabelled build cannot put one column's figures under another's heading.
pypcapon 3.12+ reports unsupported on this interpreter rather than failed — it didn't fail, it was never attempted, and the reader is owed the cause rather than the consequence.Two tables, because there are two questions
table-versions.rsttable.rstdefault, pooled across the matrixThe per-version table has to be absolute: its columns came from one host differing only in the interpreter, which is exactly when absolute times are comparable. A ratio across two interpreters is neither engine speed nor interpreter speed. The ratio table stays because absolute times can't be reproduced across machines — which is the state the current README table is in.
A missing column is held to the same standard as a missing row
Five images mean five chances for something outside these pins to break. None of them ends the run:
run.shrecords the reason per version, and the reporting pass turns it into a column of--with the reason attached rather than a narrower table. The exit status says which happened —0all measured,3tables written but a version lost,1no table at all,2bad arguments. Amake benchthat got one column of five is not a failure and not a success either.A real failure mode the first full run exposed
tsharkcrashed on pass 3 after ~2,000 spawns (TSharkCrashException, retcode 255). The exception propagated and the run produced no output at all — six working engines lost because the seventh hiccupped once.Now contained at two levels, both counted: a failed extraction is discarded (up to 5 per engine per pass) and the pass continues; a failed pass costs that engine that pass, is marked with what was lost, and an engine becomes
*not measured*only if every pass failed. Same principle in the Dockerfile — apypcapbuild failure becomes that row's reason instead of aborting the image.Two things are never tolerated, since tolerating them means publishing a number that isn't true: the escalated
EngineWarningmeaning the engine fell back to pcapkit's own parser, and a capture that yielded no packets.Every measurement asserts the engine that actually ran
The harness's most important property. A missing or unusable engine only warns and falls back to pcapkit's parser, so a run that looks successful can be timing the wrong thing three times under three names.
type(extractor.engine).__engine_name__is checked on every extraction, against drivers derived fromExtractor.__engine__rather than hand-written.Apple Silicon
No
--platformoverride needed. Every pinned digest is a multi-arch index includinglinux/arm64/v8;pcap-ct,libpcap,dpkt,scapy,pyshark,pypcapfileare arch-independent,lxmlhas aarch64 wheels for cp310–cp314, andpypcap— the only compile — has no arch-specific code (itssetup.pybranches onsys.maxsize > 2**32, not the machine).If
--platform linux/amd64is forced,run.shwarns on stderr and threads the caveat into the emitted table, because whoever reads the table later is not whoever saw the stderr.Sample output
From a real 1,000 × 3 run — but on 3.11 only, before the matrix existed. Kept because the numbers are real; it is not what the matrix now emits, which is the per-version table alongside this one:
Both snippets are plain-docutils-safe — no Sphinx-only roles — so they go straight into
README.rst. Nothing host-identifying: the point of containerising is that the host stops mattering.Two results that contradict the current README
Worth a look before anything is pasted:
pypcapfilemeasures faster thandpkt(0.0548 vs 0.0616), reversing the published ordering. Plausible on a 6-packet capture where it does the least work, but it contradicts the existing figures.pysharkis 89×, not the ~123× implied today (24.68/0.2004). Same order of magnitude, different hardware.Verification
116 self-tests (was 55) ·
shellcheckclean · both emitted snippets parse under plain docutils with zero messages · a six-version--quickmatrix built and measured every column including the 3.15 rc · a forced-missing column produced a marked column, a version-level note, unblamed engines and exit 3.Honest caveat: no full-fidelity matrix run has been made. Every run was
--quick(50 × 2), which proves the harness and both tables but is not publishable. The full 1,000 × 3 matrix should also run on the machine whose figures the README quotes — its Test Environment block names an M2 Pro, and numbers from another host aren't comparable with it.Three bugs found while verifying, all fixed
--pythons 3.1matched the3.10row and started buildingpy3.1— a typo silently becoming a run.pypcapbuild's pip output as that engine's reason: a line containing**kwargs, GNU'scannot find `pcap.h, a trailingimp_or a trailing::each turn the pasted table into a visible error block on GitHub.render_textcalled a partly-measured version "not measured" while printing its figures three lines above.Notes
examples/benchmark/README.mdis nowREADME.rst, for consistency with the project README —git mv, so history follows. RST doesn't nest inline markup, so the 17 places where bold wrapped a code span are now one or the other; verified through the parsed doctree, since docutils keeps the stray backticks as text without warning..gitignorechange needed —out/already coversexamples/benchmark/out/.