(ci) Add workflows for testing and benchmarking best-of-n - #248
(ci) Add workflows for testing and benchmarking best-of-n#248s-akhtar-baig wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds best-of-n support to the benchmark pipeline, rewrites algorithm setup around explicit lm and orchestrator instances, switches benchmark execution to always-async inference, and updates CI coverage and dependency constraints for the new benchmark path. ChangesAsync Best-of-N Benchmark Support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarking/benchmark.py (1)
267-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeparate expected-pass-at-one outputs from standard runs.
Both modes currently share the same filename, but one writes
responseand the other writesresponses/log_probs. Reusing a prior run can read missing columns or skip incompatible rows.Suggested fix
else: alg_str = alg.value + if eval_expected_pass_at_one: + alg_str += "-expected-pass-at-one" output_file = os.path.join(Also applies to: 374-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarking/benchmark.py` around lines 267 - 280, The filename construction in the benchmark output path is still shared between standard runs and expected-pass-at-one runs, which can mix incompatible JSONL schemas. Update the filename logic in the benchmark output builder so the branch that produces `response` is separated from the branch that produces `responses`/`log_probs`, using a distinct suffix or identifier in the `alg_str`/`output_file` generation to avoid reusing incompatible prior outputs. Apply the same change anywhere the output filename is built for this benchmark flow, including the other referenced block that uses the same naming pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarking/benchmark.py`:
- Around line 388-394: Handle the flattened ParticleFilteringResult shape in the
benchmarking path: the branch that calls scaling_alg.ainfer(...,
return_response_only=False) should not read result.responses_lst for
particle-filtering. Update the y_full construction to use the flattened fields
returned by ParticleFiltering.ainfer, especially responses and log_weights_lst,
so the expected-pass-at-one computation sees the actual samples instead of an
empty/mismatched payload. Use the ParticleFilteringResult handling in
benchmarking/benchmark.py and the scaling_alg.ainfer call site to locate the
fix.
- Around line 430-431: In the expected-pass-at-one scoring path,
row["log_probs"] is still a Python list, so _softmax() will fail when it
performs NumPy operations. Update the logic around the row["correct"]
calculation to convert row["log_probs"] to a NumPy array before calling
_softmax(), using the existing result.log_weights_lst / cached log_probs flow as
the source, so the benchmark evaluation can complete without TypeError.
- Line 458: The benchmark teardown is closing lm in a separate asyncio.run after
the event loop that created its cached aiohttp sessions has already been torn
down. Move the await lm.close() call into run_benchmark() itself, ideally in a
finally block near the existing benchmark loop, so the lm sessions are released
before the loop exits and use the run_benchmark function as the main place to
locate this fix.
---
Outside diff comments:
In `@benchmarking/benchmark.py`:
- Around line 267-280: The filename construction in the benchmark output path is
still shared between standard runs and expected-pass-at-one runs, which can mix
incompatible JSONL schemas. Update the filename logic in the benchmark output
builder so the branch that produces `response` is separated from the branch that
produces `responses`/`log_probs`, using a distinct suffix or identifier in the
`alg_str`/`output_file` generation to avoid reusing incompatible prior outputs.
Apply the same change anywhere the output filename is built for this benchmark
flow, including the other referenced block that uses the same naming pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a80d6f3b-3208-4ad2-b1e1-dd520241e72e
📒 Files selected for processing (3)
.github/workflows/benchmark.yaml.github/workflows/e2e.yamlbenchmarking/benchmark.py
| result = await scaling_alg.ainfer( | ||
| lm, x["problem"], n, return_response_only=False | ||
| ) | ||
| y_full = { | ||
| "responses": result.responses_lst[-1], | ||
| "log_probs": result.log_weights_lst[-1], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle the flattened ParticleFilteringResult shape.
Line 392 expects responses_lst, but ParticleFiltering.ainfer(..., return_response_only=False) returns flattened responses and log_weights_lst, so particle-filtering expected-pass-at-one skips every example.
Suggested fix
result = await scaling_alg.ainfer(
lm, x["problem"], n, return_response_only=False
)
+ if hasattr(result, "responses"):
+ responses = result.responses
+ log_probs = result.log_weights_lst
+ else:
+ responses = result.responses_lst[-1]
+ log_probs = result.log_weights_lst[-1]
y_full = {
- "responses": result.responses_lst[-1],
- "log_probs": result.log_weights_lst[-1],
+ "responses": responses,
+ "log_probs": log_probs,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = await scaling_alg.ainfer( | |
| lm, x["problem"], n, return_response_only=False | |
| ) | |
| y_full = { | |
| "responses": result.responses_lst[-1], | |
| "log_probs": result.log_weights_lst[-1], | |
| } | |
| result = await scaling_alg.ainfer( | |
| lm, x["problem"], n, return_response_only=False | |
| ) | |
| if hasattr(result, "responses"): | |
| responses = result.responses | |
| log_probs = result.log_weights_lst | |
| else: | |
| responses = result.responses_lst[-1] | |
| log_probs = result.log_weights_lst[-1] | |
| y_full = { | |
| "responses": responses, | |
| "log_probs": log_probs, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarking/benchmark.py` around lines 388 - 394, Handle the flattened
ParticleFilteringResult shape in the benchmarking path: the branch that calls
scaling_alg.ainfer(..., return_response_only=False) should not read
result.responses_lst for particle-filtering. Update the y_full construction to
use the flattened fields returned by ParticleFiltering.ainfer, especially
responses and log_weights_lst, so the expected-pass-at-one computation sees the
actual samples instead of an empty/mismatched payload. Use the
ParticleFilteringResult handling in benchmarking/benchmark.py and the
scaling_alg.ainfer call site to locate the fix.
| p = _softmax(row["log_probs"]) | ||
| row["correct"] = np.dot(p, c) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'def _softmax|x_shifted = x - np.max|_softmax\(row\["log_probs"\]\)' \
its_hub/core/algorithms/particle_gibbs.py benchmarking/benchmark.pyRepository: Red-Hat-AI-Innovation-Team/its_hub
Length of output: 1041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== benchmark.py context ==\n'
sed -n '380,445p' benchmarking/benchmark.py
printf '\n== log_probs references ==\n'
rg -n -C3 'log_probs' benchmarking its_hub
printf '\n== softmax definition ==\n'
sed -n '68,80p' its_hub/core/algorithms/particle_gibbs.pyRepository: Red-Hat-AI-Innovation-Team/its_hub
Length of output: 6205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== log_weights_lst references ==\n'
rg -n -C4 'log_weights_lst|responses_lst|class .*Result|return_response_only|ainfer\(' its_hub benchmarking
printf '\n== surrounding ainfer implementations ==\n'
for f in $(rg -l 'log_weights_lst|responses_lst' its_hub benchmarking); do
echo "--- $f ---"
sed -n '1,260p' "$f" | sed -n '1,260p'
doneRepository: Red-Hat-AI-Innovation-Team/its_hub
Length of output: 50390
Convert log_probs to a NumPy array before _softmax().
row["log_probs"] is a Python list here (result.log_weights_lst[-1] / cached log_probs), so _softmax() will raise TypeError when it subtracts np.max(x) and abort expected-pass-at-one evaluation.
Suggested fix
- p = _softmax(row["log_probs"])
+ p = _softmax(np.asarray(row["log_probs"], dtype=float))
row["correct"] = np.dot(p, c)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| p = _softmax(row["log_probs"]) | |
| row["correct"] = np.dot(p, c) | |
| p = _softmax(np.asarray(row["log_probs"], dtype=float)) | |
| row["correct"] = np.dot(p, c) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarking/benchmark.py` around lines 430 - 431, In the
expected-pass-at-one scoring path, row["log_probs"] is still a Python list, so
_softmax() will fail when it performs NumPy operations. Update the logic around
the row["correct"] calculation to convert row["log_probs"] to a NumPy array
before calling _softmax(), using the existing result.log_weights_lst / cached
log_probs flow as the source, so the benchmark evaluation can complete without
TypeError.
|
|
||
| return rows, budget_timings | ||
|
|
||
| rows, budget_timings = asyncio.run(run_benchmark()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'asyncio\.run\(run_benchmark\(\)\)|asyncio\.run\(lm\.close\(\)\)|WeakKeyDictionary|async def close|_get_session' \
benchmarking/benchmark.py its_hub/core/lms/openai_lm.pyRepository: Red-Hat-AI-Innovation-Team/its_hub
Length of output: 3551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' its_hub/core/lms/openai_lm.py
printf '\n--- benchmark excerpt ---\n'
sed -n '390,500p' benchmarking/benchmark.pyRepository: Red-Hat-AI-Innovation-Team/its_hub
Length of output: 12404
Close LM sessions on the benchmark loop. benchmarking/benchmark.py:458-482 tears down the event loop that created the cached aiohttp sessions, then closes lm in a second asyncio.run(). Move await lm.close() into run_benchmark() (for example in finally) so the sessions are released before that loop exits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarking/benchmark.py` at line 458, The benchmark teardown is closing lm
in a separate asyncio.run after the event loop that created its cached aiohttp
sessions has already been torn down. Move the await lm.close() call into
run_benchmark() itself, ideally in a finally block near the existing benchmark
loop, so the lm sessions are released before the loop exits and use the
run_benchmark function as the main place to locate this fix.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@s-akhtar-baig can we rebase? |
2 participants
Description
Closes: #184
Checklist
uv run pytest)uv run ruff check its_hub/)Summary by CodeRabbit
Summary by CodeRabbit
New Features
best-of-nto both benchmarking and end-to-end test coverage alongsideself-consistency.Bug Fixes
Chores
datasetsversion in the optionalresearchdependency set.