fix: handle missing training config - #376
Conversation
dosubot
Bot
added
size:XS
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
=======================================
Coverage 84.43% 84.43%
=======================================
Files 104 104
Lines 6110 6110
=======================================
Hits 5159 5159
Misses 951 951 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The fix is correct as far as it goes, and I verified the new test is not vacuous: reverting the two lines gives AttributeError: 'NoneType' object has no attribute 'get' on test_exec_v2_none_config, with 26/26 green at c94879e. But I think this should be fixed one line higher up, because the current shape leaves a second, related bug in place on the exact lines being touched.
Please hoist normalize_config above these reads
impl is read from the raw config seven lines before RunDPTrain.normalize_config() runs. The dargs Argument("impl", ...) declares alias=["backend"], and dargs only resolves aliases inside normalize_value(). So:
{'backend': 'pytorch'} -> .get('impl', 'tensorflow') == 'tensorflow'
{'backend': 'pytorch'} -> normalize_config(...)['impl'] == 'pytorch'
A caller who writes the documented alias gets a silent TensorFlow run: no --pt, TF init-frozen-model flags, frozen_model.pb. Nothing raises.
Normalizing first makes config=None, {}, and the backend alias all take one path, and lets the four .get(key, literal) reads become plain lookups — which also removes the hand-duplicated copies of "tensorflow" and "dp" that currently shadow the schema defaults and can silently drift from them:
# Normalize first so config=None, an empty dict, and aliases such as
# "backend" for "impl" all follow the same default path.
config = RunDPTrain.normalize_config(
ip["config"] if ip["config"] is not None else {}
)
impl = config["impl"]
dp_command = config["command"].split()
assert impl in ["tensorflow", "pytorch"]
if impl == "pytorch":
dp_command.append("--pt")
finetune_args = config["finetune_args"]
train_args = config["train_args"]I applied exactly this on top of your branch and ran it. All 26 tests in tests/op/test_run_dp_train.py pass unchanged, including your new test_exec_v2_none_config, and the alias case is fixed:
config={'backend': 'pytorch'} -> impl=pytorch dp_command=['dp', '--pt']
config={'impl': 'pytorch'} -> impl=pytorch dp_command=['dp', '--pt']
config={} -> impl=tensorflow dp_command=['dp']
config=None -> impl=tensorflow dp_command=['dp']
There is no ordering constraint preventing this: nothing between the fallback assignment and the normalize_config call consumes config.
For context on why the file drifted into this shape - the fallback line is the original code, from 1ca9aaa (2022-02-05). The two raw reads were added underneath it much later, impl in f2e1d59 (#207, 2024-03-30) and command in 8fb287e (#257, 2024-09-03), each inserted directly below a fallback the author did not use. f2e1d59 added finetune_args = config.get(...) on the local variable in the very same hunk, so the file has been inconsistent with itself since that commit. f2e1d59 also introduced the backend alias, meaning the alias has never worked on this read path.
The backend alias gap is not reachable through the CLI, since normalize_args() resolves the whole input tree before submit.py reads config["train"]["config"]. It bites direct/library callers of the OP - which is precisely the caller this PR's new test is written to represent, so it seems worth closing in the same change rather than leaving a second trap on the same lines.
Also: the added comment is inaccurate
# Read all optional values from the normalized fallback so config=None
# follows the same default path as an empty configuration.
config is not normalized at that point - normalize_config runs afterwards. And it is not "all optional values": training_args() declares 15 optional arguments, of which 4 are read here and the other 11 are read later via bracket access, after normalization. This matters more than wording, because the belief the comment plants - that these reads already went through dargs defaulting and alias resolution - is exactly what makes the backend bug above invisible. Hoisting the call would make the comment true.
One last note, unrelated to the change itself: config=None is not reachable via the CLI either. "config": null under train is rejected at submit (ArgumentTypeError ... requires <dict> but None is not a dict), and omitting the key yields a full default dict. So this is a defensive fix for library callers, which is fine and does bring the file in line with the idiom already used in run_lmp.py, collect_run_caly.py and run_caly_dp_optim.py - just worth stating plainly in case the issue implied a live production crash.
| dp_command = ip["config"].get("command", "dp").split() | ||
| # Read all optional values from the normalized fallback so config=None | ||
| # follows the same default path as an empty configuration. | ||
| impl = config.get("impl", "tensorflow") |
There was a problem hiding this comment.
This read happens before RunDPTrain.normalize_config() on line 195, and Argument("impl", ...) declares alias=["backend"], which dargs resolves only inside normalize_value(). So {"backend": "pytorch"} yields impl == "tensorflow" here, --pt is never appended, and the run silently uses TensorFlow.
Hoisting normalize_config above this block fixes the None case, the alias, and the duplicated "tensorflow"/"dp" defaults in one move. I ran that variant against your branch: 26/26 tests pass and {"backend": "pytorch"} correctly produces ['dp', '--pt'].
| config = ip["config"] if ip["config"] is not None else {} | ||
| impl = ip["config"].get("impl", "tensorflow") | ||
| dp_command = ip["config"].get("command", "dp").split() | ||
| # Read all optional values from the normalized fallback so config=None |
There was a problem hiding this comment.
Two inaccuracies here. config is not normalized at this point - normalize_config() runs on line 195, after these reads; these are raw .get() calls with hand-written literal defaults. And it is not "all optional values": 4 of the 15 optional arguments declared in training_args() are read here, the other 11 later via bracket access.
Worth correcting because this is the precise misconception behind the alias bug on line 188 - if the values really did come from the normalized config, backend would work.
Read training command defaults from the local fallback configuration so RunDPTrain accepts config=None consistently. Closes deepmodeling#351 Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
c94879e to
67e3943
Compare
2 participants
Summary
RunDPTrain.execute()withconfig=NoneTests
PYTHONPATH=tests python -m unittest -v tests.op.test_run_dp_train.TestRunDPTrain.test_exec_v2_none_configgit diff --checkCloses #351
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh