Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughUpdates the reproducibility notebook to run 100 validated prompts across multiple thresholds and refactors the causal intervention script to compute three rho metrics (rho_channels, rho_tokens, rho_current), propagate per-layer min_dt constraints, modify clamping logic, and add a --debug flag (and apply_clamp_hooks debug parameter); tests adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Evaluator
participant HookSystem
participant Model
participant Stats
CLI->>Evaluator: start run (thresholds, N_SAMPLES, debug)
Evaluator->>Model: load model & prompts
Evaluator->>HookSystem: install clamp hooks (layer_indices, target_rho, debug)
HookSystem->>Stats: compute_rho_stats(dt, A)
Stats-->>HookSystem: rho_channels, rho_tokens, rho_current
HookSystem->>HookSystem: compare dt vs per-layer min_dt (dt_floor)
alt dt < dt_floor or clamp needed
HookSystem->>Stats: _clamp_dt_to_target(..., min_dt_required)
Stats-->>HookSystem: dt_scaled, rho_before, rho_after
HookSystem->>Model: apply scaled dt to layer
end
Evaluator->>Model: run forward passes, collect metrics
Evaluator->>CLI: write results (repro CSVs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
scripts/run_causal_intervention.py (1)
126-127: Prefix unused unpacked variables with underscore.
rho_channels_beforeandrho_tokens_beforeare unpacked but never used in this function. Prefix them with underscores to signal intent and silence the linter.Suggested fix
- rho_channels_before, rho_tokens_before, rho_current_before = _compute_rho_stats(dt, A) + _rho_channels_before, _rho_tokens_before, rho_current_before = _compute_rho_stats(dt, A)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_causal_intervention.py` around lines 126 - 127, The unpacking of _compute_rho_stats(dt, A) returns three values but the first two (rho_channels_before, rho_tokens_before) are never used; rename them to start with underscores (e.g., _rho_channels_before, _rho_tokens_before) in the assignment to indicate they are intentionally unused and silence the linter while leaving rho_current_before unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/run_causal_intervention.py`:
- Around line 126-127: The unpacking of _compute_rho_stats(dt, A) returns three
values but the first two (rho_channels_before, rho_tokens_before) are never
used; rename them to start with underscores (e.g., _rho_channels_before,
_rho_tokens_before) in the assignment to indicate they are intentionally unused
and silence the linter while leaving rho_current_before unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 229a9a27-34a7-445f-8293-f397ed6915ab
📒 Files selected for processing (3)
notebooks/06_Causal_Intervention_Reproduction.ipynbscripts/run_causal_intervention.pytests/test_run_causal_intervention.py
DaviBonetto
force-pushed
the
codex/fix/colab-causal-repro-notebook
branch
from
e746929 to
f70ed3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
notebooks/06_Causal_Intervention_Reproduction.ipynb (1)
66-68: Consider removing machine-specific fallback pathsLine 66-68 embeds personal/local absolute directories. For a public Colab repro notebook, this adds brittle environment coupling;
REPO_ROOT_OVERRIDEplus clone fallback is usually enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notebooks/06_Causal_Intervention_Reproduction.ipynb` around lines 66 - 68, Remove the hard-coded, machine-specific absolute Paths from the repo-root fallback list (the entries like Path("/content/drive/MyDrive/Research - code - copy") and similar) and rely on REPO_ROOT_OVERRIDE plus the existing clone/fallback logic; update the code that builds the candidate Paths (where the list of Path(...) is defined) to omit personal/local directories so the notebook uses REPO_ROOT_OVERRIDE or the repo clone fallback only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notebooks/06_Causal_Intervention_Reproduction.ipynb`:
- Around line 50-52: The notebook currently sets REPO_REF to a mutable branch
and only clones into CLONE_DIR without enforcing that an existing repository is
checked out to that ref; update the clone/update logic (the block that handles
cloning and existing .git in CLONE_DIR) to: validate that REPO_REF is a specific
immutable ref (preferably a tag or commit hash) or allow an explicit override,
and when CLONE_DIR contains a .git, fetch and checkout REPO_REF (or reset to the
commit) so the working tree matches REPO_REF; reference the symbols REPO_REF and
CLONE_DIR and modify the existing clone/update block to run git fetch && git
checkout <REPO_REF> (or git reset --hard <commit>) to guarantee reproducible
code.
In `@scripts/run_causal_intervention.py`:
- Around line 158-159: The hooks created by make_hook close over the loop-scoped
variables A and min_abs_A, causing all registered hooks (created per layer via
make_hook) to see the final loop values; fix this by capturing those tensors in
the hook's local scope when the hook is created—e.g., pass A and min_abs_A into
make_hook (or into the inner hook) as parameters or bind them as default
arguments so the inner function uses the per-layer copies rather than the outer
loop variables; update references in the hook (the function named hook inside
make_hook and any uses of A/min_abs_A elsewhere in that closure) to use the
captured local names (e.g., A_local, min_abs_A_local) so each registered hook
uses the correct layer-specific tensors.
---
Nitpick comments:
In `@notebooks/06_Causal_Intervention_Reproduction.ipynb`:
- Around line 66-68: Remove the hard-coded, machine-specific absolute Paths from
the repo-root fallback list (the entries like
Path("/content/drive/MyDrive/Research - code - copy") and similar) and rely on
REPO_ROOT_OVERRIDE plus the existing clone/fallback logic; update the code that
builds the candidate Paths (where the list of Path(...) is defined) to omit
personal/local directories so the notebook uses REPO_ROOT_OVERRIDE or the repo
clone fallback only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f752ab96-a9e1-4495-8b2e-41ec8845617e
📒 Files selected for processing (3)
notebooks/06_Causal_Intervention_Reproduction.ipynbscripts/run_causal_intervention.pytests/test_run_causal_intervention.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_run_causal_intervention.py
Comment on lines
+50
to
+52
| "REPO_URL = \"https://github.com/DaviBonetto/spectralguard.git\"\n", | ||
| "REPO_REF = \"codex/fix/colab-causal-repro-notebook\"\n", | ||
| "CLONE_DIR = Path(\"/content/spectralguard\")\n", |
There was a problem hiding this comment.
Reproducibility risk: mutable branch ref and no ref enforcement on existing clone
Line 51 pins to a branch name, and Line 83-100 does not ensure CLONE_DIR is checked out to that ref when .git already exists. This can silently run different code across sessions.
🔧 Proposed hardening
-REPO_REF = "codex/fix/colab-causal-repro-notebook"
+# Prefer an immutable commit SHA for reproducibility.
+REPO_REF = "PUT_COMMIT_SHA_HERE"
@@
- if not (CLONE_DIR / ".git").exists():
+ if not (CLONE_DIR / ".git").exists():
print(f"Cloning {REPO_URL} ({REPO_REF}) into {CLONE_DIR} ...")
subprocess.run(
@@
check=True,
)
+ else:
+ # Ensure we are on the intended ref even when reusing an existing clone.
+ subprocess.run(
+ ["git", "-C", str(CLONE_DIR), "fetch", "origin", REPO_REF, "--depth", "1"],
+ check=True,
+ )
+ subprocess.run(
+ ["git", "-C", str(CLONE_DIR), "checkout", REPO_REF],
+ check=True,
+ )Also applies to: 83-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notebooks/06_Causal_Intervention_Reproduction.ipynb` around lines 50 - 52,
The notebook currently sets REPO_REF to a mutable branch and only clones into
CLONE_DIR without enforcing that an existing repository is checked out to that
ref; update the clone/update logic (the block that handles cloning and existing
.git in CLONE_DIR) to: validate that REPO_REF is a specific immutable ref
(preferably a tag or commit hash) or allow an explicit override, and when
CLONE_DIR contains a .git, fetch and checkout REPO_REF (or reset to the commit)
so the working tree matches REPO_REF; reference the symbols REPO_REF and
CLONE_DIR and modify the existing clone/update block to run git fetch && git
checkout <REPO_REF> (or git reset --hard <commit>) to guarantee reproducible
code.
Switch the intervention check to the repo's aggregated rho metric, avoid float32 saturation near 1.0, and use a monotonic search so the clamp stays finite and respects the post-condition.
Keep the hook aligned with _compute_rho_stats returning channels, tokens, and current rho. This removes the unpacking crash in the Colab reproduction path while preserving the clamp logic.
Raise the causal reproduction notebook to the paper's Table 3 configuration with 100 validated prompts and the full threshold grid. This removes the mini-run mismatch so the rerun can be compared directly against the published table.
Replace the brittle global scale search with the analytical per-channel minimum step size derived from A. This makes the causal intervention follow the diagonal operator bound more closely and avoids false clamp failures on long prompts.
DaviBonetto
force-pushed
the
codex/fix/colab-causal-repro-notebook
branch
from
f70ed3d to
3474071
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features
Chores
Tests
Bug Fixes