GH-16717: Fix DeepLearning params modification issue - #16794
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes intermittent DeepLearning checkpoint-resume failures caused by training-time resolution of AUTO parameters (notably _distribution) leaking back into the caller’s Parameters instance and later triggering checkpoint “parameter changed” validation.
Changes:
- Clone
Model.Parametersinto both_input_parmsand_parmsinsideModelBuilderconstructors to prevent mutation of the caller-provided parameters object during training. - Add a DeepLearning regression test covering checkpoint resumption when
distribution=AUTOis involved.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
h2o-core/src/main/java/hex/ModelBuilder.java |
Clones incoming parameters to avoid mutating the caller’s parms object during model training. |
h2o-algos/src/test/java/hex/deeplearning/DeepLearningTest.java |
Adds a regression test to ensure checkpoint resume works when AUTO distribution gets resolved during training. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maurever
left a comment
maurever
left a comment
There was a problem hiding this comment.
LGTM, thanks @tomasfryda.
|
It seems like Java 8 Smoke is a legit fail.. |
|
@valenad1 yes, I'm looking at it now. The issue seems to be now in the GLM. The root cause is that we expect side effects from For example: parms = new ModelParms();
parms.family = AUTO;
Model m = new Model();
m.init(parms);
assert parms.family != AUTO;
// and more importantly
assert parms != m._input_parms;Which I think is a wrong behavior since we introduced the |
c8fcb19 to
79b6ae3
Compare
|
The exception type delivered by Job.get() depended on a race: if the job failed before get() attached to the barrier, the original exception arrived wrapped in a generic RuntimeException, otherwise it was rethrown as-is. This made tests expecting H2OIllegalArgumentException from checkpoint restarts fail intermittently.
|
#16717