Housekeeping: KNOWN_ISSUES #16, GPU legend grouping fix, run_container.sh extra binds

[zachtheyek]

Closes #111

Summary

Three small housekeeping items (PR-01 of the v1 push):

1. KNOWN_ISSUES.md entry Welcome to Aetherscan Discussions! #16 — documents the pip dependency-resolver error (pydot 3.0.4 vs pyparsing 2.4.7) printed during singularity/apptainer build of aetherscan.def. The conflict is pre-existing in the NGC tensorflow:25.02-tf2-py3 base image, nothing in requirements-container.txt touches either package, Aetherscan never imports pydot, and the built image is fully functional. Status: won't fix (upstream base image); documented. Follows the exact Symptom/Cause/Impact/Workaround/Status format of entries 1-15.

2. GPU legend grouping fix (src/aetherscan/monitor/monitor.py) — the combined usage+memory legend used ncol=min(4, n_gpus*2); matplotlib fills legends column-major, so with 5 GPUs the 10 entries landed in 4 columns as 3/3/2/2 with memory entries continuing mid-column under usage entries. Now each metric's handle/label lists are padded to a whole number of columns with invisible handles (Line2D([], [], alpha=0), empty label), with cols_per_metric = ceil(n_gpus/3) (caps the legend at 3 rows) and ncol = 2*cols_per_metric — usage entries occupy their own column(s) and memory entries always start a fresh column (5 GPUs -> columns of 3,2 | 3,2).

3. utils/run_container.sh extra binds — new AETHERSCAN_EXTRA_BINDS env var: comma-separated host paths, each bound 1:1 and appended to the standard bind list, so CSV inference on blpc3 becomes AETHERSCAN_EXTRA_BINDS=/datag ./utils/run_container.sh ... instead of relying on the runtime's native SINGULARITY_BIND passthrough (which continues to work and remains additive). Documented in the script header, the README raw-.h5 inference example, and the README .env example block.

No cli.py changes, so the README CLI Reference blocks are untouched.

Verification

There is no test suite yet (tracked separately as the upcoming testing-suite PR), so no pytest claims — here is exactly what was verified and how:

Legend fix (local matplotlib script, no cluster needed): reproduced the exact pre-fix and post-fix legend construction (same twinx axes, per-GPU solid usage + dashed memory lines, same label format) for n_gpus in {1, 2, 4, 5, 6, 8} and eyeballed the saved PNGs:

• Old logic, 5 GPUs: columns of 3/3/2/2 with ... :0 (Memory) appearing directly under :3/:4 (Usage) — the reported bug, reproduced.
• New logic: n=1 -> 1 row usage | memory; n=2 -> one usage column, one memory column; n=4 -> 2,2 | 2,2; n=5 -> 3,2 | 3,2; n=6 -> 3,3 | 3,3; n=8 -> 3,3,2 | 3,3,2. In every case memory entries start a fresh column and never share a column with usage entries.
• Edge cases checked in code: the branch only runs when gpu_data is non-empty (no zero division); only the GPU loop adds labeled artists to these axes, so entry counts never exceed the padded slot count; a GPU missing one metric just gets extra invisible padding.

Extra binds (blpc3, this branch checked out in the cluster repo):

$ AETHERSCAN_EXTRA_BINDS=/datag ./utils/run_container.sh ls /datag
blpd0 blpd1 ... blpd18 collate_mb incoming index.html lost_and_found mattl pipeline ptest public srt users

$ ./utils/run_container.sh ls /            # plain run, no regression
bin boot datax dev ... workspace

$ SINGULARITY_BIND=/datag ./utils/run_container.sh ls /datag   # passthrough still works
blpd0 blpd1 blpd10 ...

The cluster repo was restored to master afterward (clean tree). pre-commit run --all-files passes; bash -n utils/run_container.sh passes; all commits GPG-signed.

Deferred validation: none — nothing in this PR is bla0-only. The production resource plot renders on pipeline shutdown, so the next real cluster run will exercise the new legend path end-to-end.

AI assistance disclosure

Implemented with Claude Code (implementation, documentation, and verification, per AI_POLICY.md). All changes were verified as described above.

🤖 Generated with Claude Code

[claude]

Claude finished @zachtheyek's task in 1m 6s —— View job

---

PR Review: Housekeeping fixes

• Gather context and review PR diff
• Review KNOWN_ISSUES.md entry Welcome to Aetherscan Discussions! #16
• Review GPU legend grouping fix in monitor.py
• Review run_container.sh extra binds
• Review README.md changes
• Post final review

---

Clean, well-scoped housekeeping PR. All three changes are correct and well-documented. A few observations below:

KNOWN_ISSUES.md entry #16

Follows the existing format exactly. The rationale is sound — the conflict is upstream in the NGC base image, pydot is unused, and the warning against "fixing" pyparsing (risking NGC-pinned packages) is a good defensive note. No issues.

GPU legend grouping fix (monitor.py)

The logic is correct. Key things I verified:

• No zero-division risk: the branch is guarded by if gpu_data and self.num_gpus > 0 (line 536), so len(gpu_data) >= 1 and cols_per_metric >= 1.
• Padding is always non-negative: each GPU contributes at most one entry to each axis, so len(lines1) <= len(gpu_data) <= slots — the slots - len(lines1) expression can never go negative.
• Asymmetric GPU metrics: if some GPUs report usage but not memory (or vice versa), the lists just get more invisible padding, which is correct.
• The Line2D([], [], alpha=0) approach is a well-known matplotlib pattern for invisible legend spacers.

One minor thought: nrows (line 595) is computed but only used to derive slots. The expression slots = nrows * cols_per_metric could be simplified to slots = math.ceil(len(gpu_data) / cols_per_metric) * cols_per_metric, but since nrows makes the intent clearer (explicitly showing the 3-row cap reasoning), the current form is better for readability. No change needed.

run_container.sh extra binds

Well-implemented:

• The IFS=',' read -ra pattern correctly splits comma-separated values.
• [[ -z $extra_path ]] && continue gracefully handles trailing commas or empty entries.
• Extra paths are intentionally not validated for existence before binding — this is the right call; the container runtime will produce a clear error if a path doesn't exist, which is more useful than a shell-level pre-check that might mask the real failure mode.
• The refactor from inline --bind flags to a BIND_ARGS array is a clean structural improvement that makes the extra-binds append natural.
• The header comment documenting that SINGULARITY_BIND/APPTAINER_BIND passthrough is still additive is a helpful detail.

README.md

Both additions (.env example block entry and the raw .h5 inference example update) are clear and consistent with the existing documentation style.

Overall

LGTM. No bugs, no security concerns, no style issues. The verification methodology described in the PR body is thorough for a repo without a test suite.

[zachtheyek]

@claude please perform a second-pass code review of this PR. Re-review the current diff for any remaining correctness bugs, security issues, or significant quality problems, and post your findings. If the diff is clean, please say so explicitly. (Final verification round before merge.)

[claude]

Second-Pass Code Review

I've reviewed the current diff for correctness bugs, security issues, and quality problems. The diff is clean — no issues found.

Details by change:

1. KNOWN_ISSUES.md entry 16 — Pure documentation; follows the existing format exactly. No concerns.

2. GPU legend grouping fix (monitor.py) — Verified correctness:

• The new logic is guarded by if gpu_data and self.num_gpus > 0: (line 534), so len(gpu_data) >= 1 always holds — no division-by-zero risk in math.ceil(len(gpu_data) / 3).
• slots = nrows * cols_per_metric is always >= len(gpu_data), so slots - len(lines1) and slots - len(lines2) are always non-negative (no crash from negative repeat count). If a GPU is missing one metric (line 556/568 conditional), the padding just grows slightly — harmless.
• Line2D([], [], alpha=0) with empty-string labels is the standard matplotlib idiom for invisible legend padding. The math import and Line2D import are both used.

3. run_container.sh extra binds — Verified correctness and security:

• IFS=',' read -ra correctly splits on commas; the [[ -z $extra_path ]] && continue guard handles trailing commas or double-commas gracefully.
• Paths are user-supplied env vars (same trust level as the existing AETHERSCAN_*_PATH vars), so no privilege escalation surface.
• The array-based BIND_ARGS approach correctly handles paths with spaces (quoted expansion via "${BIND_ARGS[@]}").
• The existing SINGULARITY_BIND/APPTAINER_BIND passthrough is unaffected (documented in the header comment).

4. README.md — Documentation additions are accurate and consistent with the implementation.

No correctness bugs, no security issues, no quality problems. LGTM for merge.

[claude]

Release note draft — Improved

The GPU resource plot legend now correctly groups usage and memory entries into separate columns, fixing a layout issue where memory labels could appear under usage labels on systems with 5 or more GPUs. Additionally, the container launcher (run_container.sh) now supports an AETHERSCAN_EXTRA_BINDS environment variable for binding extra host paths into the container (e.g. AETHERSCAN_EXTRA_BINDS=/datag), making it easier to run inference on data outside the standard directories without relying on runtime-specific bind passthrough.

Draft for the changelog; edit or drop as needed.