fix: missing log in multitask training#5382
fix: missing log in multitask training#5382iProzd wants to merge 2 commits intodeepmodeling:masterfrom
Conversation
📝 WalkthroughWalkthroughRefactored multi-task logging in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| train_results[_key] = log_loss_train( | ||
| loss, more_loss, _task_key=_key | ||
| ) | ||
| valid_results[_key] = log_loss_valid(_task_key=_key) | ||
| if self.rank == 0: | ||
| log.info( | ||
| format_training_message_per_task( | ||
| batch=display_step_id, | ||
| task_name=_key + "_trn", | ||
| rmse=train_results[_key], | ||
| learning_rate=cur_lr, | ||
| ) | ||
| ) | ||
| if valid_results[_key]: | ||
| valid_results[_key] = log_loss_valid(_task_key=_key) | ||
| if self.rank == 0: | ||
| log.info( | ||
| format_training_message_per_task( | ||
| batch=display_step_id, | ||
| task_name=_key + "_val", | ||
| rmse=valid_results[_key], | ||
| learning_rate=None, | ||
| task_name=_key + "_trn", | ||
| rmse=train_results[_key], | ||
| learning_rate=cur_lr, | ||
| ) | ||
| ) | ||
| if valid_results[_key]: | ||
| log.info( | ||
| format_training_message_per_task( | ||
| batch=display_step_id, | ||
| task_name=_key + "_val", | ||
| rmse=valid_results[_key], | ||
| learning_rate=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The indentation change moves valid_results[_key] = log_loss_valid(...) (and both log messages) into the same inner block as train_results[_key] = .... Previously, validation collection/logging happened at a broader scope; if that outer condition isn’t always entered (e.g., loss not computed/filtered for a task), this will skip validation results and logs for that task, potentially reintroducing the ‘missing log’ behavior. Consider restoring valid_results assignment and the rank==0 logging block to the prior scope (keeping only the intended logging fix), so per-task validation/logging isn’t coupled to that inner conditional.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5382 +/- ##
==========================================
- Coverage 80.40% 80.38% -0.02%
==========================================
Files 812 812
Lines 83476 83477 +1
Branches 4050 4050
==========================================
- Hits 67115 67106 -9
- Misses 15139 15147 +8
- Partials 1222 1224 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
maybe add a simple UT to count number of columns in lcurve.out? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/tests/pt/test_multitask.py`:
- Line 76: Rename the ambiguous comprehension variable `l` in the list
comprehension inside test_multitask.py to a clearer name (e.g., `line`) so ruff
E741 is avoided; update the expression `data_lines = [l for l in lines if not
l.startswith("#")]` to use the new identifier (`line`) and ensure any adjacent
references in the same scope match the rename.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: efcb3235-4e4e-4d57-8f12-b610de5b78a7
📒 Files selected for processing (1)
source/tests/pt/test_multitask.py
| len(cols_for_model), 0, f"No lcurve columns found for {mk}" | ||
| ) | ||
| # data line column count should match header | ||
| data_lines = [l for l in lines if not l.startswith("#")] |
There was a problem hiding this comment.
Rename ambiguous variable l to pass CI.
The variable name l is flagged by ruff (E741) as ambiguous since it can be confused with 1 or I. As per coding guidelines, ruff check . must pass before committing.
Proposed fix
- data_lines = [l for l in lines if not l.startswith("#")]
+ data_lines = [line for line in lines if not line.startswith("#")]📝 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.
| data_lines = [l for l in lines if not l.startswith("#")] | |
| data_lines = [line for line in lines if not line.startswith("#")] |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 76-76: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/tests/pt/test_multitask.py` at line 76, Rename the ambiguous
comprehension variable `l` in the list comprehension inside test_multitask.py to
a clearer name (e.g., `line`) so ruff E741 is avoided; update the expression
`data_lines = [l for l in lines if not l.startswith("#")]` to use the new
identifier (`line`) and ensure any adjacent references in the same scope match
the rename.
fix missing log during multitask training, resulting from #4850 .
Summary by CodeRabbit
Refactor
Tests