Fix Perf Bug and add Perf Testing to CI - #160
Merged
Merged
Conversation
In `_ground_rule`, the node-head branch called
`check_all_clause_satisfaction` once per head grounding, inside
`for head_grounding in groundings[head_var_1]`. The check does not take
`head_grounding`, so the only way its result could vary between
iterations is if the loop body mutated state the check reads. The body
has exactly one such mutation, `_add_node`, guarded by
`add_head_var_node_to_graph` — and that flag is only ever set on the
branch that assigns `groundings[head_var_1]` a single-element list. When
the mutation can fire, the loop runs once, so there is nothing to differ.
The check is therefore loop-invariant, and the work it repeated scaled
with the number of head groundings: O(clauses x groundings) re-evaluated
K times per rule firing.
Run it once, before the loop, and enter the loop only if it passes. Guard
it behind a non-empty head-grounding check so a rule with no heads does
no work. `satisfaction` is not read after this block, so initializing it
to False changes nothing observable.
The edge-head branch keeps its per-iteration check: that one reads
`temp_groundings`, which is rebuilt for each head pair and is genuinely
not invariant.
Measured on a Friends/owns graph with many candidate heads, identical
observable output in both variants:
people before after speedup
100 0.232 s 0.169 s 1.37x
500 2.320 s 1.164 s 1.99x
1,000 9.282 s 3.660 s 2.54x
2,000 37.127 s 6.089 s 6.10x
Applied identically to interpretation.py, interpretation_fp.py and
interpretation_parallel.py; the latter still differs from the first only
in the parallel= decorator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ColtonPayne
force-pushed
the
Bruno-Faster-Pyreason
branch
from
August 14, 2026 19:40
29b7123 to
92666df
Compare
Ruff 0.16.0 (2026-07-23) expanded its default rule set from 59 rules to 413 — the first change to that default since v0.1.0. The workflow installs ruff unpinned and pyproject.toml sets no `select`, so the repo silently inherited all 413 and now reports ~289 findings (SIM, UP, RUF, I001, BLE, EXE, PIE, FURB, C4) on code that was green in May. This is not specific to any one branch: main has not run CI since 2026-05-13 and fails the same way, so every PR opened after 0.16.0 landed was broken on arrival regardless of its contents. Pin to 0.15.22, the last release carrying the old E4/E7/E9/F default, so lint results depend on the code rather than on when the job happened to run. Note that 0.16 also dropped 18 rules from the defaults, E741 among them, so the existing per-file-ignore stays necessary under this pin. Adopting the broader rule set remains an option, but it needs an explicit `select` plus a deliberate one-time cleanup, reviewed on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No longer stores the pokec-2k fixtures, it now generates them in real time
Make the perf workflow's behavior deliberate instead of incidental, and cut its dependency on an external host. Gating - Both jobs now carry symmetric conditions whose only filter is the workflow_dispatch size selector. pokec-10k already ran on every PR, but via an `if:` whose first clause short-circuits true for all non-dispatch events -- accidental, and left over from 25fe35a ("FOR TESTING"). pokec-2k was additionally excluded from the nightly cron for no stated reason. - The two sizes run as parallel jobs, so the gate costs the longer of them (~11 min), while python-package-version-test.yml on the same PR takes 29-35 min. Perf is never the critical path, and runner minutes are free on public repos, so 2k rides along at no wall-clock cost. - 10k goes to --repeats 3, matching 2k, so its median is measured the same way the banked baseline was. - Timeouts drop from 45/300 min to 20/30 min: real ceilings with ~3x headroom rather than placeholders that would let a hung job burn five hours. - Add concurrency cancel-in-progress for pull_request only, so superseded PR runs stop while main and nightly history stays complete. Fixtures - Commit both fixtures gzipped (76 KB + 509 KB) and gunzip them in CI. This removes the actions/cache steps and the two curl calls to snap.stanford.edu: no cold-miss download, no third-party outage in the gate's path, and fork PRs -- which cannot write to the Actions cache -- behave like branch PRs. - Regenerated from the SNAP dumps with the current generate_fixtures.py. The 2k GraphML is byte-identical to the one previously committed at 25fe35a; the 10k has never been in git before. Both match the documented reference counts (2k: 18,315 friend / 1,203 hasPet / 5 customers; 10k: 121,716 / 6,204 / 14). - Compressing with the no-timestamp flag keeps the archives byte-reproducible from identical GraphML. - .gitignore now excludes only the unpacked .graphml build artifact. README: describe the gate as intended, document why both sizes run per PR, why LFS is the wrong tool at this size, and how to regenerate.
Follow-up on review notes that survived the previous commit. Editable install - The perf jobs installed the package non-editable, but run_bench.py's _spawn_child prepends the repo root to the child's PYTHONPATH, so the child always imports the checkout. The site-packages copy was never imported -- confirmed locally where site-packages held 3.5.1 while the checkout was 3.6.0 and the child still resolved to the checkout. Switching to -e removes the discrepancy and states that the checkout is what gets measured, which is the correct thing for a perf gate to measure. run_bench.py - Reject --repeats below 1 in the parent. Previously it reached statistics.median on an empty list and raised StatisticsError, which named neither the flag nor the cause. - A child that exits 0 having printed nothing now fails with that sentence rather than a bare IndexError from splitlines()[-1]. Both run_bench.py paths were unreachable in CI; they only bite someone driving the script by hand. Two earlier notes are moot: restore-keys, because the actions/cache steps and the SNAP downloads are gone, and cancel-in-progress, which the previous commit added. A paths: filter is deliberately not added -- see the README.
ColtonPayne
changed the title
kmukherji
approved these changes
Aug 24, 2026
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
3 participants
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.
For this tests, the implementation consistently produced identical observable outputs and scaled substantially better as number of nodes inferred by the rule increased. Median speedup rose from 1.37× at 100 people to 6.10× at 2,000 people
Test Results: