Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
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 |
Document the training, exploration, and labeling download interface and cover its iteration and artifact filters. Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
d375da3 to
21d0b9a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
=======================================
Coverage 85.98% 85.98%
=======================================
Files 105 105
Lines 6930 6930
=======================================
Hits 5959 5959
Misses 971 971 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The two donwload typo fixes are welcome - they have been on the public fullcli page since d6c8dfa (#135, 2023-02-10). And the new test is real coverage, not a restatement: I reverted --no-check-point to store_true and it fails, and changed --iterations to nargs=1 and it errors.
Some claims I checked and am explicitly not raising, so they do not get re-opened:
- The layout
results/iter-000000/<step>/<input-or-output>/<artifact>is correct for the flags your example uses._item_pathsplits the item on/then on--, soiter-000000--prep-run-train/output/modelsbecomesresults/iter-000000/prep-run-train/output/models. - The
--no-check-pointsentence is correct despite the double negation:action="store_false"means the flag is absent ->no_check_point=True->chk_pnt=True-> completed items skipped. - Every artifact name in the example command exists in
op_download_setting.
Three things to fix.
1. Closes #58 overclaims - please make it Refs #58. Issue #58 asks for three things, and its second bullet is "the result files of lmp exploration, including the input and output files of lmp exploration tasks". That one is not merely undocumented, it is structurally unavailable: in dpgen2/superop/prep_run_lmp.py, expl_task_grp is an InputParameter, not an Artifact; PrepRunLmp's input artifacts are only ["models"]; and task_paths (which carries in.lammps and conf.lmp) is produced by the inner prep-lmp step and consumed by run-lmp's slices without ever becoming a superop-level output. So it can never appear in op_download_setting["prep-run-explore"], which has zero input definitions. Auto-closing #58 would bury a real, still-open feature gap. The download UI itself shipped in #74 (2022-09-27), #106 (2022-12-28) and #135 (2023-02-10); none of them referenced #58, which is the only reason it is still open.
2. The prose was not checked against the code it describes. Three separate inaccuracies, one root cause - see the inline comments on the bullet list and on the --iterations line. A fourth, from the same cause, does not need its own thread: line 50 says "every successful iteration", but the filter is per step (if step is None or step["phase"] != "Succeeded": continue in download_dpgen2_artifacts_by_def), so a half-failed iteration still yields its succeeded steps. Your own main.py edit in this same commit says "all successful steps" - that wording is the right one.
3. Worth considering while you are in there (not blocking, listed so they are not lost):
- The section never mentions
-k/--keys, which is a supported flag with its own test. It takes the other code path,download_dpgen2_artifacts, whose layout is different: pluralinputs/outputs, with all of a step's artifacts merged into one directory and no per-artifact leaf. A reader who uses--keysgets no guidance from this section and a different tree than the one documented. - The new test uses only long flags, while
test_dldandtest_watchin the same file pin the short ones. I renamed-dto-D, leaving--step-definitionsintact, and all five tests still passed - so-i,-d,-land-nremain unexercised by anything, even though the help text you edited advertises-i 0-8 8 9 -d prep-run-train/input/init_data. Also worth assertingparsed.keys is None(that is the condition inmain.pythat actually routes todownload_by_defrather thandownload, i.e. the dispatch this test is named for) and the defaultno_check_point is True. - Line 39, just above the new section, already says "the existing files are automatically skipped if one sets
dflow_config["archive_mode"] = None". That is dflow'sskip_exists, a completely different mechanism from thedone-marker checkpoint your new paragraph describes. The two now sit 26 lines apart with nothing distinguishing them.
| Files are organized below `results/iter-000000/<step>/<input-or-output>/<artifact>`. Existing completed downloads are skipped by default; pass `--no-check-point` to request them again. The corresponding result groups are: | ||
|
|
||
| - training: models, learning curves, logs, and generated scripts; | ||
| - exploration: trajectories, model deviations, logs, and extra outputs; | ||
| - labeling: input configurations, labeled data, logs, and extra outputs. |
There was a problem hiding this comment.
This hand-written list does not match op_download_setting, and it will drift further.
collect-datais missing entirely. It is the fourth entry in the registry (.add_output("iter_data")- the accumulated training dataset, arguably the most useful thing to download), and--list-supportedprints it. Line 50 says a no-filter run fetches "every supported artifact", so a reader will take these bullets as the inventory and never learncollect-data/output/iter_dataexists.- The exploration bullet is already wrong against master. 1f21e7c (Add PLUMED CV-aware candidate filtering #372, 2026-09-06) added
.add_output("plm_output")toprep-run-explore. That commit is not an ancestor of this branch's base, the files do not overlap, so git merges it silently and the bullet lands incomplete with nothing to flag it. - The training bullet lists outputs only, dropping
init_models,init_dataanditer_data- while the labeling bullet does include its input (confs-> "input configurations"). Same registry, two different conventions in adjacent lines. Note your ownmain.pyexample in this commit usesprep-run-train/input/init_data, an artifact these bullets never mention.
The section already tells the reader to run --list-supported first, which is the authoritative and self-maintaining answer. Consider naming the four step groups and leaving the artifact enumeration to that command, rather than copying a list that has to be re-checked on every registry change.
There was a problem hiding this comment.
Addressed in d51a5d8. The page now names the four step groups including collect-data and delegates the artifact inventory to --list-supported. It also says successful steps, distinguishes key-based download layout and dflow skip_exists from the done-marker checkpoint, and changes Closes #58 to Refs #58. Synced current master; six CLI parser tests pass, including short flags and defaults.
Agent: dot
|
|
||
| ```bash | ||
| dpgen2 download input.json WFID \ | ||
| --iterations 0-2 \ |
There was a problem hiding this comment.
0-2 is a half-open range: it expands to iterations 0 and 1, not 0, 1 and 2.
expand_idx in dpgen2/entrypoint/common.py does ret += range(int(range_str[0]), int(range_str[1]), step), and Python's range excludes the upper bound. This is also why the help text has to write the awkward -i 0-8 8 9 to cover iterations 0 through 9.
This page already knows how to say it - the resubmit section reads "0<=id<41, note that 41 is not included". The new section is the only range example on the page without that caveat, and a reader copying this command will believe they downloaded the last iteration when they did not.
There was a problem hiding this comment.
Addressed in d51a5d8: the example explicitly says 0-2 selects iterations 0 and 1, excludes 2, and uses 0-3 to include it. Six parser tests pass, now also covering short flags, default checkpoints and the keys=None dispatch precondition.
Agent: dot
Summary
Tests
Refs #58
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh