fix: log multirun failures to job files#3227
Conversation
|
Hi @suzunn! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thanks. overall looks good. I'll take a closer look when it hits my queue. |
There was a problem hiding this comment.
Pull request overview
This PR ensures that exceptions raised during a basic multirun sweep are logged while the per-job logging configuration is still active, so each failed job’s *.log file contains actionable failure details.
Changes:
- Add a small internal helper in
run_job()to emit the active exception to file handlers only (avoiding extra console tracebacks). - Extend the partial-failure multirun regression test to assert the failed job log contains the exception.
- Add a bugfix news fragment for the user-visible behavior change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hydra/core/utils.py |
Logs job exceptions to per-job file handlers inside the job’s logging context before storing the exception in JobReturn. |
tests/test_basic_sweeper.py |
Extends the multirun partial-failure test to validate per-job log content for the failing job. |
news/3215.bugfix |
Documents the bugfix in release notes. |
|
Thanks, this is in the right direction. |
|
Also, please accept the CLA, I cannot accept the change with it. |
|
Hi, are you planning to finish this? |
dbaeb70 to
757e2a5
Compare
|
Yes — sorry for the delay. Just pushed an update:
Both tests pass locally. Will sort out the CLA next. |
757e2a5 to
c8cf4a4
Compare
|
Thanks for following up and adding the multiple-failure coverage. This now covers the case I was concerned about. The change looks good. |
Problem
Failed jobs in a basic multirun sweep were captured in JobReturn without being written while the job logging configuration was active. This left per-job log files empty and made failures other than the first one difficult to diagnose.
Solution
I added a focused file-handler path that records the active exception in the job log without emitting another traceback to the console. I also extended the partial-failure regression test and added a bugfix news fragment.
Fixes #3215
Testing
.venv/Scripts/python -m pytest tests/test_basic_sweeper.py tests/test_basic_launcher.py tests/test_callbacks.py -q(42 passed)git diff --check