PML-387 & PML-389: fixed user path in documentation#281
Conversation
…tive path from user
CassNot
changed the title
|
@ben9871 , I just updated my branch to add a new fix for PML-389 that was just detected @claude review this fix and my PR |
|
Claude finished @CassNot's task in 2m 21s —— View job Review: PR #281 — PML-387 (nbsphinx paths) + PML-289 (legacy redirects)
Nice work — both changes are well-structured, the docstrings match the file's existing numpydoc style, and factoring the shared redirect HTML into 🔴 Bug:
|
| docname | stub written at (buggy) | should be |
|---|---|---|
api_reference/api/merlin.algorithms.feed_forward |
.../api/merlin.algorithms.html |
.../api/merlin.algorithms.feed_forward.html |
api_reference/api/merlin.algorithms.layer |
.../api/merlin.algorithms.html |
.../api/merlin.algorithms.layer.html |
api_reference/api/merlin.algorithms.loss |
.../api/merlin.algorithms.html |
.../api/merlin.algorithms.loss.html |
Two consequences:
- The stub is written at the wrong path, so the actual old URL (
.../merlin.algorithms.feed_forward.html) still 404s — exactly what PML-289 is meant to fix. - Collisions overwrite each other — every
merlin.algorithms.*page collapses to a singlemerlin.algorithms.html, so only the last one written survives (as junk).
Note target_url on line 356 uses the raw docname string, so the target is correct — only the filename is wrong. The fix is to build the filename by string concatenation instead:
legacy_path = output_path / f"{docname}.html"The depth = docname.count("/") / relative_prefix logic is unaffected and stays correct (I verified reproduced_papers/index → ../0.4/reproduced_papers/index.html resolves properly, and depth-0 pages too).
🟢 PML-387 (doctree -d fix) — looks good
docs/build_multiversion.py:411,429-430 — passing an explicit per-version -d doctree dir inside the exported checkout is a sound way to keep nbsphinx image artifacts relative to the source tree, and you've verified it on merlinquantum.ai. Minor observations (non-blocking):
checkout_pathis already unique per version, so the extra/ version_nameon the doctree path is redundant (harmless).- The doctrees live in the temp checkout that's cleaned up with the
TemporaryDirectory, so there's no cache reuse between runs — expected for this script, just noting it.
🟡 Minor notes (optional)
- HTML escaping in
_redirect_page—target_url/link_textare interpolated into attributes without escaping. Docnames won't contain"in practice, so this is only a robustness nit. docnamesfor redirects — you correctly use only the latest version's docnames, which is the right behavior for landing users on current content. Worth a one-line comment noting a page removed in the latest version won't get a redirect (acceptable).
Overall: solid fix. The only thing I'd block on is the with_suffix issue, since it breaks redirects for every dotted-name page (the whole API reference). Once that's a plain f"{docname}.html", this looks ready.
· branch generating-0.4-doc
CassNot
changed the title
|
It should be fixed now @claude review |
|
Claude finished @CassNot's task in 38s —— View job Re-review: PML-387 + PML-389 —
|
| docname | stub written at (now) | target_url |
|---|---|---|
api_reference/api/merlin.algorithms.feed_forward |
.../api/merlin.algorithms.feed_forward.html ✓ |
../../0.4/api_reference/api/merlin.algorithms.feed_forward.html ✓ |
api_reference/api/merlin.algorithms.layer |
.../api/merlin.algorithms.layer.html ✓ |
../../0.4/.../merlin.algorithms.layer.html ✓ |
No more collisions, and each old URL resolves to its correct versioned target. The explanatory comment on lines 348–354 is a nice touch — it documents exactly why with_suffix() was wrong, which will save the next person from reintroducing it.
I re-verified the surrounding logic is unaffected:
- Path-traversal guard (line 358) still works —
output_path.resolve()is inlegacy_path.resolve().parentsfor both nested (reproduced_papers/index) and top-level (changelog) docnames, and rejects a leading... depth/relative_prefix(lines 361–363) —count("/")is unaffected by dots;reproduced_papers/index→../0.4/reproduced_papers/index.htmland depth-0 pages →0.4/<name>.htmlboth resolve correctly.indexskip (line 346) correctly defers towrite_latest_redirect.
Verdict
The one blocking issue from my previous review is resolved. Both fixes (PML-387 doctree -d, PML-389 legacy redirects) look correct and ready to merge.
The two earlier minor notes remain optional and non-blocking:
- HTML escaping of
target_url/link_textin_redirect_page(docnames won't contain"in practice). - Redirects only cover pages present in the latest version (correct behavior; a page removed in the latest version won't get a redirect).
Nice work. 👍
· branch generating-0.4-doc
2 participants
Summary
When building the doc on the server, it was using my path for the images in the notebook and there were errors displaying them.
Now, the multiversion builder now passes a per-version -d doctree directory inside the exported tag checkout. That keeps nbsphinx image artifacts relative to the source tree, so Sphinx copies them into _images instead of rendering /Users/.../.doctrees/nbsphinx/... paths.
I changed these lines, compiled and sent to the server and it is now fixed on merlinquantum.ai !
Related Issue
PML-387
Type of change
Proposed changes
The multiversion builder now passes a per-version -d doctree directory inside the exported tag checkout. That keeps nbsphinx image artifacts relative to the source tree, so Sphinx copies them into _images instead of rendering /Users/.../.doctrees/nbsphinx/... paths.
How to test / How to run
Go on merlinquantum.ai
Checklist
SPHINXOPTS="-W --keep-going -n" make -C docs clean htmlthe docs are built without any warning or errors.