[None][fix] Narrow bare except clause and use identity check for None#12041
[None][fix] Narrow bare except clause and use identity check for None#12041edenfunf wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request applies two Python code quality fixes to a scaffolding example file: replacing an improper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/scaffolding/run_majority_vote_aime24.py (1)
78-90:⚠️ Potential issue | 🔴 Critical
--concurrencywithout--static_with_benchmarkstill crashes here.If the caller passes
--concurrencyalone, this branch runs buttask_collection_typesis only assigned insideif args.static_with_benchmark:. Line 89 then uses it unconditionally, so the script fails withUnboundLocalErrorbefore any requests execute.Example fix
if args.static_with_benchmark or args.concurrency: if args.concurrency is None: args.concurrency = 1 + task_collection_types = {} if args.static_with_benchmark: task_collection_types = {"token_counter": GenerationTokenCounter}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/scaffolding/run_majority_vote_aime24.py` around lines 78 - 90, The branch handling concurrency/static benchmarking sets task_collection_types only when args.static_with_benchmark is true, causing an UnboundLocalError when --concurrency is provided alone; ensure task_collection_types is always defined before it is passed to async_scaffolding_benchmark by initializing it (e.g., to None or an empty dict) before the if args.static_with_benchmark block, keep the existing assignment to GenerationTokenCounter inside that block, and then call async_scaffolding_benchmark(llm, task_collection_types, requests, ...) using the guaranteed variable; adjust only the setup around args.concurrency, task_collection_types, and the requests list (ScaffoldingBenchRequest/prompts) so the code no longer errors when --concurrency is used without --static_with_benchmark.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@examples/scaffolding/run_majority_vote_aime24.py`:
- Around line 78-90: The branch handling concurrency/static benchmarking sets
task_collection_types only when args.static_with_benchmark is true, causing an
UnboundLocalError when --concurrency is provided alone; ensure
task_collection_types is always defined before it is passed to
async_scaffolding_benchmark by initializing it (e.g., to None or an empty dict)
before the if args.static_with_benchmark block, keep the existing assignment to
GenerationTokenCounter inside that block, and then call
async_scaffolding_benchmark(llm, task_collection_types, requests, ...) using the
guaranteed variable; adjust only the setup around args.concurrency,
task_collection_types, and the requests list (ScaffoldingBenchRequest/prompts)
so the code no longer errors when --concurrency is used without
--static_with_benchmark.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af76fd52-5794-467d-9036-45a4f5af4451
📒 Files selected for processing (1)
examples/scaffolding/run_majority_vote_aime24.py
cc9a3e8 to
fb4376d
Compare
…_majority_vote_aime24.py - Change bare except to except (ValueError, TypeError) to avoid catching KeyboardInterrupt and SystemExit, which prevents clean process termination during long evaluations. - Change == None to is None per PEP 8 to avoid potential issues with custom __eq__ implementations. Fixes: NVIDIA#12040 Signed-off-by: 許元豪 <146086744+edenfunf@users.noreply.github.com>
fb4376d to
3153f6a
Compare
|
/bot run |
|
PR_Github #38367 [ run ] triggered by Bot. Commit: |
|
PR_Github #38367 [ run ] completed with state
|
|
Hi, I’ve updated the PR title to follow the required format. |
|
Hi @edenfunf , I'll take care of CI failures if it's due to some unstable machine, and will ping you if there is real issue related to the PR. So don't worry about these failure messages sent by bot. |
|
/bot run --disable-fail-fast |
|
PR_Github #38434 [ run ] triggered by Bot. Commit: |
Background / Motivation
examples/scaffolding/run_majority_vote_aime24.pycontains two code quality issues that can cause unexpected runtime behavior:except:clause (line 112) catches all exceptions includingKeyboardInterruptandSystemExit, making it impossible to cleanly terminate the process with Ctrl+C during long AIME24 evaluations.== Nonecomparison (line 79) violates PEP 8. The==operator can be overridden by__eq__, potentially leading to incorrect behavior with custom objects.Change Summary
except:->except (ValueError, TypeError):— these are the only exceptionsint()can raise, so this precisely catches the intended error cases while allowing system exceptions to propagate normally.== None->is None— uses identity comparison as recommended by PEP 8.Impact
Related Issue
Fixes #12040
Summary by CodeRabbit