Add UI network inspectors - #101
Conversation
…raining instructions
📝 WalkthroughWalkthroughThe PR adds typed computation contracts and figure builders for network inspectors, a Streamlit inspection UI, a CLI launcher and styling, a batch Torch training script, documentation, packaging configuration, and regression tests. ChangesNetwork inspectors
Batch Torch training
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Inspector
participant StreamlitApp
participant ModelArtifacts
participant NetworkAPI
participant Plotting
Inspector->>StreamlitApp: Select model and analysis view
StreamlitApp->>ModelArtifacts: Discover files and load predictor
StreamlitApp->>NetworkAPI: Compute likelihood comparison or manifold
NetworkAPI->>Plotting: Build figure
Plotting-->>StreamlitApp: Return figure
StreamlitApp-->>Inspector: Render analysis view
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
scripts/train_torch_models_batch.sh (1)
282-291: 🧹 Nitpick | 🔵 TrivialFail-fast batch semantics: a single training failure stops all remaining models/network IDs.
set -euo pipefailis active for the whole script. If"${cmd[@]}"at line 289 fails for one model/network-id combination, the script exits immediately and skips every remaining combination in the batch. This may be the intended behavior for reproducibility, but for long batch runs, consider whether the script should continue on failure and report a summary of failed combinations at the end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/train_torch_models_batch.sh` around lines 282 - 291, Update the batch execution around the "${cmd[@]}" invocation so a failed model/network-id combination is recorded and the loop continues processing all remaining combinations despite set -e. Track each failed combination using the existing model and net_id identifiers, then report a summary of failures after both loops complete while preserving successful execution behavior.src/lanfactory/network_inspectors/streamlit_app.py (2)
286-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the commented-out accessibility caption.
This dead code block is commented out and unused. Either restore it as active UI text or remove it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/network_inspectors/streamlit_app.py` around lines 286 - 289, Remove the commented-out accessibility caption block near the Streamlit UI code; do not restore it as active text unless the surrounding implementation explicitly requires that caption.
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to
predictorand_load_predictor's return value.
_load_predictorhas no return type annotation, andpredictorin_kde_tab/_manifold_tabhas no type annotation, even though the same callable type (Callable[[NDArray[np.float32]], Any]) is already used consistently in api.py. Add matching annotations for mypy compatibility.As per coding guidelines, "Keep the package compatible with mypy type checking."
Also applies to: 152-152, 207-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/network_inspectors/streamlit_app.py` around lines 88 - 97, Add type annotations for mypy compatibility by including the return type annotation on the _load_predictor function and type annotations on the predictor variables in _kde_tab and _manifold_tab functions. Use the consistent callable type Callable[[NDArray[np.float32]], Any] that is already established in api.py to ensure uniform type signatures across the codebase.Source: Coding guidelines
test_network_inspectors.py (1)
37-239: 🎯 Functional Correctness | 🔵 TrivialConsider filing follow-up issues for the documented bugs.
These
xfail(strict=True)tests are a good way to pin down known bugs without blocking this PR. Several of the documented issues affect correctness in ways users can hit directly: missingparameter_dfvalidation (Line 178), missingtorch_mlp_predictvalidation (Line 223), and the hardcoded 4000-row batch sizing (Line 101) can produce confusing runtime errors or silently wrong results rather than a clear message.Do you want me to help implement fixes for any of these and remove the corresponding
xfailmarkers?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test_network_inspectors.py` around lines 37 - 239, File follow-up issues for the documented bugs, prioritizing validation in kde_vs_lan_likelihoods and lan_manifold and choice-count-based batch sizing in kde_vs_lan_likelihoods. Keep the strict xfail tests as regression coverage for now; remove each marker only when its corresponding implementation fix is completed.src/lanfactory/cli/network_inspectors_ui.py (1)
19-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winForward extra CLI arguments to
streamlit run.
sys.argvis replaced entirely with["streamlit", "run", str(app_path)]. Any arguments a user passes tonetwork-inspectors-ui(for example,--server.port 8502) are silently dropped instead of reaching Streamlit. Forwardsys.argv[1:]so users can pass Streamlit configuration options through the wrapper.🔧 Proposed fix
- sys.argv = ["streamlit", "run", str(app_path)] + sys.argv = ["streamlit", "run", str(app_path), *sys.argv[1:]]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/cli/network_inspectors_ui.py` around lines 19 - 25, Update the CLI entrypoint around app_path and the sys.argv assignment to preserve and append the wrapper’s original sys.argv[1:] arguments after the Streamlit app path, ensuring options such as --server.port reach stcli.main() while retaining the existing streamlit run invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 153-172: Wrap the two multi-line shell command examples in the
"Batch Training Multiple Torch Models" section with bash code fences. The first
example demonstrates scripts/train_torch_models_batch.sh with
--training-data-base and --networks-path-base flags, and the second example
shows the same script with --training-data-folder flag. Enclose each full
multi-line command (from the script invocation through all its continued lines
with backslash) in ```bash ``` code fences to ensure proper Markdown rendering
of the shell continuation syntax.
In `@scripts/train_torch_models_batch.sh`:
- Around line 143-177: Update resolve_model_training_folder so the
missing-directory branch echoes candidate_folder instead of an empty string,
allowing the caller’s “Training data folder not found” message to include the
attempted path. Preserve the existing resolution behavior for valid directories.
- Around line 194-223: Update the validation_out assignment in the batch
validation flow to use an unambiguous plain command substitution by removing the
extra opening parenthesis after $( and its matching closing parenthesis.
Preserve the existing uv run python heredoc and validation behavior unchanged.
In `@src/lanfactory/network_inspectors/plotting.py`:
- Line 161: Split the function signature for build_manifold_figure across
multiple lines to comply with the 88-character line limit. Apply the same
multi-line formatting pattern already established in build_kde_vs_lan_figure to
maintain consistency. Ensure each resulting line stays within the 88-character
limit while preserving the function's parameters (computation:
ManifoldComputation, cfg: PlotConfig) and return type annotation (go.Figure).
In `@src/lanfactory/network_inspectors/streamlit_app.py`:
- Around line 188-198: Wrap the likelihood computation and figure construction
inside the “Run KDE vs LAN” block with try/except handling for ValueError,
matching the predictor-loading error path. Display the caught validation error
through st.error instead of allowing a raw traceback, while preserving the
existing spinner and successful comparison/figure flow.
- Around line 370-374: Add a min_value parameter to the st.number_input call for
rt_step_2c that enforces a positive value constraint (use a small positive
threshold to prevent zero or negative step values that would break the
downstream KDE grid computation).
- Around line 207-272: Update _manifold_tab so the Run Manifold computation
handles unsupported models and other expected failures without exposing a
traceback: wrap compute_lan_manifold and figure construction in a try/except,
display the exception through st.error, and return without rendering results
when computation fails. Preserve the existing validation and successful plotting
flow.
In `@tests/test_network_inspectors_plotting.py`:
- Around line 23-40: Update
test_build_kde_vs_lan_figure_returns_matplotlib_figure to close the returned
Figure after the isinstance assertion, using the existing matplotlib
figure-closing mechanism so the test does not leave build_kde_vs_lan_figure
resources open.
---
Nitpick comments:
In `@scripts/train_torch_models_batch.sh`:
- Around line 282-291: Update the batch execution around the "${cmd[@]}"
invocation so a failed model/network-id combination is recorded and the loop
continues processing all remaining combinations despite set -e. Track each
failed combination using the existing model and net_id identifiers, then report
a summary of failures after both loops complete while preserving successful
execution behavior.
In `@src/lanfactory/cli/network_inspectors_ui.py`:
- Around line 19-25: Update the CLI entrypoint around app_path and the sys.argv
assignment to preserve and append the wrapper’s original sys.argv[1:] arguments
after the Streamlit app path, ensuring options such as --server.port reach
stcli.main() while retaining the existing streamlit run invocation.
In `@src/lanfactory/network_inspectors/streamlit_app.py`:
- Around line 286-289: Remove the commented-out accessibility caption block near
the Streamlit UI code; do not restore it as active text unless the surrounding
implementation explicitly requires that caption.
- Around line 88-97: Add type annotations for mypy compatibility by including
the return type annotation on the _load_predictor function and type annotations
on the predictor variables in _kde_tab and _manifold_tab functions. Use the
consistent callable type Callable[[NDArray[np.float32]], Any] that is already
established in api.py to ensure uniform type signatures across the codebase.
In `@test_network_inspectors.py`:
- Around line 37-239: File follow-up issues for the documented bugs,
prioritizing validation in kde_vs_lan_likelihoods and lan_manifold and
choice-count-based batch sizing in kde_vs_lan_likelihoods. Keep the strict xfail
tests as regression coverage for now; remove each marker only when its
corresponding implementation fix is completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac5f072b-5544-4729-9d60-e83008105aab
📒 Files selected for processing (14)
.gitignoreREADME.mdpyproject.tomlscripts/train_torch_models_batch.shsrc/lanfactory/cli/network_inspectors_ui.pysrc/lanfactory/network_inspectors/__init__.pysrc/lanfactory/network_inspectors/api.pysrc/lanfactory/network_inspectors/contracts.pysrc/lanfactory/network_inspectors/plotting.pysrc/lanfactory/network_inspectors/streamlit_app.pysrc/lanfactory/network_inspectors/styles.csstest_network_inspectors.pytests/test_network_inspectors_api.pytests/test_network_inspectors_plotting.py
| resolve_model_training_folder() { | ||
| local candidate_folder="$1" | ||
| local model="$2" | ||
|
|
||
| if [[ ! -d "$candidate_folder" ]]; then | ||
| echo "" | ||
| return | ||
| fi | ||
|
|
||
| # Preferred: pickle shards directly in the provided folder. | ||
| if find "$candidate_folder" -maxdepth 1 -type f -name '*.pickle' | grep -q .; then | ||
| echo "$candidate_folder" | ||
| return | ||
| fi | ||
|
|
||
| # Common layout: one subfolder per model. | ||
| if [[ -d "$candidate_folder/$model" ]] && find "$candidate_folder/$model" -maxdepth 1 -type f -name '*.pickle' | grep -q .; then | ||
| echo "$candidate_folder/$model" | ||
| return | ||
| fi | ||
|
|
||
| # Fallback: if exactly one immediate subfolder contains pickle shards, use it. | ||
| local subdir_count | ||
| subdir_count="$(find "$candidate_folder" -mindepth 1 -maxdepth 1 -type d | wc -l | tr -d ' ')" | ||
| if [[ "$subdir_count" == "1" ]]; then | ||
| local only_subdir | ||
| only_subdir="$(find "$candidate_folder" -mindepth 1 -maxdepth 1 -type d | head -n 1)" | ||
| if find "$only_subdir" -maxdepth 1 -type f -name '*.pickle' | grep -q .; then | ||
| echo "$only_subdir" | ||
| return | ||
| fi | ||
| fi | ||
|
|
||
| echo "$candidate_folder" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Uninformative error message when the candidate folder is missing.
resolve_model_training_folder echoes "" at line 148 when $candidate_folder does not exist. At line 257 this produces Training data folder not found for model 'x': with a blank path, which does not tell the user what path was actually attempted. Echo the candidate path instead so the failing path shows up in the error message.
🐛 Proposed fix
if [[ ! -d "$candidate_folder" ]]; then
- echo ""
+ echo "$candidate_folder"
return
fiAlso applies to: 256-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/train_torch_models_batch.sh` around lines 143 - 177, Update
resolve_model_training_folder so the missing-directory branch echoes
candidate_folder instead of an empty string, allowing the caller’s “Training
data folder not found” message to include the attempted path. Preserve the
existing resolution behavior for valid directories.
| local validation_out | ||
| if ! validation_out="$((uv run python - "$folder" <<'PY' | ||
| import glob | ||
| import os | ||
| import pickle | ||
| import sys | ||
|
|
||
| folder = sys.argv[1] | ||
| files = sorted(glob.glob(os.path.join(folder, "*.pickle"))) | ||
| if not files: | ||
| print("ERR_NO_PICKLES") | ||
| raise SystemExit(2) | ||
|
|
||
| with open(files[0], "rb") as f: | ||
| obj = pickle.load(f) | ||
|
|
||
| if not isinstance(obj, dict): | ||
| print("ERR_NOT_DICT") | ||
| raise SystemExit(3) | ||
|
|
||
| keys = set(obj.keys()) | ||
| required = {"lan_data", "lan_labels"} | ||
| if not required.issubset(keys): | ||
| print("ERR_BAD_KEYS") | ||
| print(",".join(sorted(keys))) | ||
| raise SystemExit(4) | ||
|
|
||
| print("OK_KEYS") | ||
| PY | ||
| ))"; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the $(( command-substitution ambiguity flagged by shellcheck.
Line 195 writes validation_out="$((uv run python - "$folder" <<'PY' ... ))". The double opening parenthesis after $( is unintentional: the goal is a plain command substitution, not a nested subshell or arithmetic expression. Shellcheck reports this as SC1102 (error level) because shells disambiguate $(( differently: some parse it as arithmetic and fail, others fall back to command substitution. Remove the redundant parentheses so the intent is unambiguous.
🐛 Proposed fix
- if ! validation_out="$((uv run python - "$folder" <<'PY'
+ if ! validation_out="$(uv run python - "$folder" <<'PY'
import glob
import os
import pickle
import sys
@@
print("OK_KEYS")
PY
-))"; then
+)"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local validation_out | |
| if ! validation_out="$((uv run python - "$folder" <<'PY' | |
| import glob | |
| import os | |
| import pickle | |
| import sys | |
| folder = sys.argv[1] | |
| files = sorted(glob.glob(os.path.join(folder, "*.pickle"))) | |
| if not files: | |
| print("ERR_NO_PICKLES") | |
| raise SystemExit(2) | |
| with open(files[0], "rb") as f: | |
| obj = pickle.load(f) | |
| if not isinstance(obj, dict): | |
| print("ERR_NOT_DICT") | |
| raise SystemExit(3) | |
| keys = set(obj.keys()) | |
| required = {"lan_data", "lan_labels"} | |
| if not required.issubset(keys): | |
| print("ERR_BAD_KEYS") | |
| print(",".join(sorted(keys))) | |
| raise SystemExit(4) | |
| print("OK_KEYS") | |
| PY | |
| ))"; then | |
| local validation_out | |
| if ! validation_out="$(uv run python - "$folder" <<'PY' | |
| import glob | |
| import os | |
| import pickle | |
| import sys | |
| folder = sys.argv[1] | |
| files = sorted(glob.glob(os.path.join(folder, "*.pickle"))) | |
| if not files: | |
| print("ERR_NO_PICKLES") | |
| raise SystemExit(2) | |
| with open(files[0], "rb") as f: | |
| obj = pickle.load(f) | |
| if not isinstance(obj, dict): | |
| print("ERR_NOT_DICT") | |
| raise SystemExit(3) | |
| keys = set(obj.keys()) | |
| required = {"lan_data", "lan_labels"} | |
| if not required.issubset(keys): | |
| print("ERR_BAD_KEYS") | |
| print(",".join(sorted(keys))) | |
| raise SystemExit(4) | |
| print("OK_KEYS") | |
| PY | |
| )"; then |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 195-195: Shells disambiguate
(SC1102)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/train_torch_models_batch.sh` around lines 194 - 223, Update the
validation_out assignment in the batch validation flow to use an unambiguous
plain command substitution by removing the extra opening parenthesis after $(
and its matching closing parenthesis. Preserve the existing uv run python
heredoc and validation behavior unchanged.
Source: Linters/SAST tools
| if st.button("Run KDE vs LAN", use_container_width=True): | ||
| with st.spinner("Computing likelihoods..."): | ||
| comparison = compute_kde_vs_lan_likelihoods( | ||
| parameter_df=parameter_df, | ||
| model=model, | ||
| torch_mlp_predict=predictor, | ||
| n_samples=n_samples, | ||
| n_reps=n_reps, | ||
| grid=grid_spec, | ||
| ) | ||
| fig = build_kde_vs_lan_figure(comparison, plot_cfg) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap likelihood computation in error handling, consistent with predictor loading.
The "Run KDE vs LAN" block calls compute_kde_vs_lan_likelihoods and build_kde_vs_lan_figure without a try/except. Any ValueError from validation (for example an empty or malformed parameter frame) surfaces as a raw traceback instead of a st.error message, unlike the predictor-loading path at lines 407-411.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lanfactory/network_inspectors/streamlit_app.py` around lines 188 - 198,
Wrap the likelihood computation and figure construction inside the “Run KDE vs
LAN” block with try/except handling for ValueError, matching the
predictor-loading error path. Display the caught validation error through
st.error instead of allowing a raw traceback, while preserving the existing
spinner and successful comparison/figure flow.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/lanfactory/trainers/torch_mlp.py:454
-torch.log(1 + torch.exp(-x))can overflow for large negativex(becausetorch.exp(-x)can becomeinf), producing-inf/nan. Using the numerically-stable softplus implementation avoids overflow while preserving the same value.
elif self.train_output_type == "logits":
return -torch.log(
1 + torch.exp(-self.layers[-1](x))
) # log ( 1 / (1 + exp(-x))), where x = log(p / (1 - p))
src/lanfactory/trainers/jax_mlp.py:128
-jnp.log(1 + jnp.exp(-x))can overflow for large negativexand produce-inf/nan. Prefer the numerically-stable softplus-based formulation to computelog(sigmoid(x)).
if (not self.train) and (self.train_output_type == "logits"):
x = -jnp.log(1 + jnp.exp(-x))
| if ! validation_out="$((uv run python - "$folder" <<'PY' | ||
| import glob | ||
| import os | ||
| import pickle | ||
| import sys | ||
|
|
||
| folder = sys.argv[1] | ||
| files = sorted(glob.glob(os.path.join(folder, "*.pickle"))) | ||
| if not files: | ||
| print("ERR_NO_PICKLES") | ||
| raise SystemExit(2) | ||
|
|
||
| with open(files[0], "rb") as f: | ||
| obj = pickle.load(f) | ||
|
|
||
| if not isinstance(obj, dict): | ||
| print("ERR_NOT_DICT") | ||
| raise SystemExit(3) | ||
|
|
||
| keys = set(obj.keys()) | ||
| required = {"lan_data", "lan_labels"} | ||
| if not required.issubset(keys): | ||
| print("ERR_BAD_KEYS") | ||
| print(",".join(sorted(keys))) | ||
| raise SystemExit(4) | ||
|
|
||
| print("OK_KEYS") | ||
| PY | ||
| ))"; then |
|
Cleaned up in #106 |
Summary by CodeRabbit
New Features
Documentation
Tests