judge: support score-based subtask dependencies#1192
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
WalkthroughAdds the optional Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
|
I have read the CLA Document and I hereby sign the CLA |
5e0b297 to
efdc955
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/hydrojudge/src/flow.ts (1)
98-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider Promise-chaining to maximize queue throughput.
The current topological sort schedules subtasks layer-by-layer, awaiting
Promise.all(ready.map(...))before advancing to the next graph depth. If one subtask in a layer takes significantly longer than the others, it will stall execution of the next layer. If the judge is configured withsingleTaskParallelism > 1, this will leave processing slots idle.Consider mapping each subtask to a localized Promise that directly awaits its specific dependencies. This ensures independent subtasks execute as soon as their unique prerequisites are met, maximizing queue throughput. (A quick DFS or the existing
whileloop can still be run beforehand to detect cycles).🤖 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 `@packages/hydrojudge/src/flow.ts` around lines 98 - 110, Replace the layer-wide Promise.all scheduling in the subtask execution flow with per-subtask promises that await only the promises for each subtask’s declared dependencies before invoking judgeSubtask. Preserve the existing dependency acceptance and score checks, failure handling, and cycle detection (the current traversal may remain as a preliminary validation), while allowing independent subtasks to start as soon as their own prerequisites complete.
🤖 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 `@packages/hydrojudge/src/flow.ts`:
- Line 103: Update the if_score dependency evaluation in the scored expression
so non-existent subtask IDs are ignored, matching the accepted check’s behavior.
Guard the infos[id]?.score comparison with the corresponding subtask existence
check, while preserving the requirement that existing dependencies must have a
positive score.
---
Nitpick comments:
In `@packages/hydrojudge/src/flow.ts`:
- Around line 98-110: Replace the layer-wide Promise.all scheduling in the
subtask execution flow with per-subtask promises that await only the promises
for each subtask’s declared dependencies before invoking judgeSubtask. Preserve
the existing dependency acceptance and score checks, failure handling, and cycle
detection (the current traversal may remain as a preliminary validation), while
allowing independent subtasks to start as soon as their own prerequisites
complete.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f68677e1-14a3-4cfb-9eeb-084631acb63d
📒 Files selected for processing (9)
packages/common/subtask.tspackages/common/types.tspackages/hydrojudge/src/flow.tspackages/hydrojudge/src/judge/run.tspackages/ui-default/components/monaco/schema/problemconfig.tspackages/ui-default/components/problemconfig/ProblemConfigEditor.tsxpackages/ui-default/components/problemconfig/reducer/config.tspackages/ui-default/components/problemconfig/tree/SubtaskSettings.tsxpackages/ui-default/locales/zh.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/common/types.ts
- packages/ui-default/components/monaco/schema/problemconfig.ts
- packages/hydrojudge/src/judge/run.ts
- packages/ui-default/locales/zh.yaml
- packages/ui-default/components/problemconfig/ProblemConfigEditor.tsx
- packages/common/subtask.ts
- packages/ui-default/components/problemconfig/tree/SubtaskSettings.tsx
| const accepted = (subtask.if || []).every((id) => ( | ||
| !subtasks[id] || (infos[id] && infos[id].status <= STATUS.STATUS_ACCEPTED) | ||
| )); | ||
| const scored = (subtask.if_score || []).every((id) => infos[id]?.score > 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore non-existent subtasks in if_score dependencies.
If an if_score dependency references a non-existent subtask ID, infos[id] will be undefined, causing scored to evaluate to false and unintentionally fail the subtask. In contrast, the accepted check (lines 100-101) correctly ignores non-existent subtasks (!subtasks[id] || ...).
To maintain consistent behavior and prevent typographical errors in the problem configuration from failing valid subtasks, if_score should also gracefully ignore invalid subtask IDs.
🐛 Proposed fix
- const scored = (subtask.if_score || []).every((id) => infos[id]?.score > 0);
+ const scored = (subtask.if_score || []).every((id) => !subtasks[id] || infos[id]?.score > 0);📝 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.
| const scored = (subtask.if_score || []).every((id) => infos[id]?.score > 0); | |
| const scored = (subtask.if_score || []).every((id) => !subtasks[id] || infos[id]?.score > 0); |
🤖 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 `@packages/hydrojudge/src/flow.ts` at line 103, Update the if_score dependency
evaluation in the scored expression so non-existent subtask IDs are ignored,
matching the accepted check’s behavior. Guard the infos[id]?.score comparison
with the corresponding subtask existence check, while preserving the requirement
that existing dependencies must have a positive score.
|
Maybe it's better to use an expression parser: if:
- '$1 > 0'
- '$2 > 10'
- '$3 == 0'cc @moesoha |
|
i think the expression is too complex and is not meaningful. we already have statuses on subtasks, and the status can be used to make that decision. from my point of view, |
Motivation
The existing
ifdependency is based on acceptance status. This does not cover partial-scoring checkers, where a subtask can receive a positive final score without being accepted and that score should still be able to unlock a dependent subtask.Summary
Add score-based subtask dependencies through a new
if_scoreconfiguration field.A subtask with
if_scoreis judged only when every referenced subtask has a final score greater than zero.Here, subtask 2 is judged when subtask 1 receives a positive score, including partial credit. It is skipped when subtask 1 receives zero points.
Summary by CodeRabbit
if_score, enabling subtasks to run only when selected prerequisite subtasks achieve scores greater than 0.