Repository navigation
Conversation
|
Reviewed against CI on Claimed: executable Python is unchanged in Verified — exact: I parsed both versions with Claimed: the entrypoint is Verified — correct: Claimed: Verified — each step against the source:
Claimed: Verified — correct: Claimed: the service no longer downloads from IPFS; it reads staged files. Verified — correct: the service reads Claimed: all 11 relative links resolve. Verified — by CI: the Docs job is green on One gap, not a blocker for this PR's scope: the old name and the "trains and uploads" wording are still in two other files under
The PR body flags these as out of scope, and issue No. 266 names only "the guide and service comments", so Merge order with PR No. 292: confirmed. A Not independently re-verified: an end-to-end Every checkable claim held up. Mergeable as-is. I'd prefer the two stale files above get fixed at merge, so closing No. 266 doesn't leave the public page saying the opposite of the guide. |
Files changed (2) — as of
|
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +41/-23 |
| Diff (what exactly is in this PR) | Rewrites the Overview, the "Its job is to" list, the Flow summary (now seven steps, from train-lms through the worker to submit-lm, plus a two-command example and the --gi rules), the Inputs table (adds model_base_dir; runtime is now WorkerRuntime), the file-layout note (/din/model, read-only with a read-write output subdir), the training/DP steps ("save … locally" instead of "upload"), and the Output Contract (the two returned path shapes). Renames the entrypoint to train_client_model. |
| Functionality — how & why | How: describes the real split. train-lms stages the service, architecture, genesis model and (after GI 1) the previous global model, runs the worker, and prints the local path. submit-lm picks lm_{gi}.pth / lm_{gi}_{mechanism}.pth from the manifest's DP config, uploads it, and calls submitLocalModel. Why: issue No. 266. The guide described the old fused "train and upload" function, which no longer exists, so a reader would expect train-lms to submit. |
Diff vs current develop HEAD |
None — untouched since merge-base. |
| Recommended merge proposal | Merged as-is. Every step was traced to the source (see the verification comment); links are green in CI Docs. |
| Actual merge proposal | Soon |
| Pending proposal | Merge order with PR No. 292 (update_laplace), which edits the DP sections of this file. Whichever lands second must keep both changes: No. 292's mechanism text and this PR's "save … locally" wording. Not a blocker for this PR. |
| Local merge conflict | No (against develop); Yes against PR No. 292's head |
| GitHub merge conflict | No |
cache_model_0/services/client.py
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +13/-7 |
| Diff (what exactly is in this PR) | The module docstring, the train_client_model docstring and two inline comments now say "load staged inputs, save, return the path" instead of "upload". |
| Functionality — how & why | How: no runtime change. The AST with docstrings stripped is equal to develop's. Why: the service's own description still said it uploads (issue No. 266, second bullet). |
Diff vs current develop HEAD |
None — untouched since merge-base. |
| Recommended merge proposal | Merged as-is. AST-equal to develop apart from docstrings. Note that PR No. 292 also edits this file; git merge-tree merges it cleanly. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Not in this PR: Documentation/public/services.md, Documentation/technical/manifest.md
| Field | Value |
|---|---|
| Change | None (still stale) |
| Lines | — |
| Diff (what exactly is in this PR) | Untouched. services.md:129 heading train_client_model_and_upload_to_ipfs(...) + "uploads it to IPFS", :149 manifest key; manifest.md:73 manifest key, :119 "dincli reads train_client_model_and_upload_to_ipfs". |
| Functionality — how & why | How: no code reads these names; cache_model_0/manifest.json uses train_client_model. Why it matters: Documentation/ describes develop, and the public page links to the guide this PR corrects, so the two would disagree. |
Diff vs current develop HEAD |
— |
| Recommended merge proposal | Either fix at merge (rename to train_client_model and change "uploads it to IPFS" to "saves it locally; submit-lm uploads it") or open a follow-up issue before merging, because Closes #266 would otherwise leave them untracked. |
| Actual merge proposal | Soon |
| Pending proposal | The merger chooses: fix at merge or open a follow-up issue. |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
CI on edfc0e0: CI OK green. Locally: an AST comparison of client.py against develop (equal), and every new sentence in the guide traced to dincli/cli/client.py, dincli/cli/worker.py, dincli/docker/worker/worker.py, dincli/cli/context.py and cache_model_0/manifest.json. Full detail is in the verification comment above.
Local vs. GitHub agree: both clean against develop.
umeradl
left a comment
There was a problem hiding this comment.
Approved at edfc0e0: client.py is unchanged apart from docstrings (AST comparison); every step in the guide was traced to dincli/cli/client.py, worker.py and the worker runner. Before or at merge: fix the stale train_client_model_and_upload_to_ipfs references in public/services.md and technical/manifest.md, or open a follow-up issue.
|
Thanks for reviewing this, @umeradl, and for catching the remaining stale references! This follow-up includes the public/manifest examples that the original PR description listed as out of scope. Pushed the documentation cleanup in No runtime or DP behavior changed. All 292 inline relative documentation links resolve, and the executable Python AST remains unchanged. If PR No. 292 lands first, I'll reconcile the overlap while preserving its DP changes and calibration qualifications. Fresh CI and re-review of the new head are still needed. |
|
Reviewed against CI on The PR's own work is two commits: Claimed —
The old name Claimed — Claimed — all relative links resolve. Verified: Claimed — Claimed — PR 292 reconciliation noted. Verified: the PR body flags "will need reconciliation after No. 292 lands." The current No findings. Merge-ready; branch needs |
|
Head at review:
|
| Key | Value |
|---|---|
| Change | +64/-30 lines: Overview rewritten for train/submit split; flow summary extended to 7 steps covering worker path; inputs table adds model_base_dir; training path step 1 and step 7 corrected; post_training_gaussian and post_training_laplace headings updated with experimental labels (Track A); output contract section updated with specific file paths; noise-calibration note updated |
| Pending proposal | None — accurate, consistent with source; PR 292's update_laplace section (not yet merged) will need to be preserved on rebase |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
cache_model_0/services/client.py
| Key | Value |
|---|---|
| Change | +18/-12 lines: module-level docstring and train_client_model function docstring updated to reflect train/submit split; no executable logic changed |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
Documentation/public/services.md
| Key | Value |
|---|---|
| Change | +16/-14 lines: section 3.1 renamed from train_client_model_and_upload_to_ipfs to train_client_model; description, parameter table and Returns row updated; manifest example entry name corrected |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
Documentation/technical/manifest.md
| Key | Value |
|---|---|
| Change | +14/-9 lines: "client manifest-loading path" section adds worker.py link; high-level flow extended to 7 steps; example entry name corrected from train_client_model_and_upload_to_ipfs to train_client_model; custom-service flow section rewritten as 7 steps covering worker lifecycle |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
1263921
Actual outcome — PR No. 304 merged as-is through the merge queue (second attempt)This replaces the predictions in Robbert's pre-merge review comment above with what actually happened when the PR landed. One commit on
First attempt, dropped by the queue (not this PR's fault): the first queue run, on top of PR No. 254 (run 41475597415), failed one Solidity test. The cause was a Files unchanged from the PR (4 of 4)
Verification
Local vs. GitHub agree: yes. The merge was clean, as both predicted. |
Closes #266.
Corrects the technical client-service guide and reference-service descriptions to match the current workflow:
train-lmssaves a local model artifact;submit-lmuploads it to IPFS and submits its CID on-chain.Updates the entrypoint name to
train_client_modeland explains worker paths and artifact selection. Runtime behavior and differential-privacy logic are unchanged.Validation:
git diff --checkpasses.Integration note: will need reconcilitaion after #292 lands. Preserve its DP changes alongside these wording corrections.