Conversation
fazelehh
force-pushed
the
feature/pet-recipe
branch
from
August 19, 2026 06:14
172f691 to
692a9bd
Compare
fazelehh
added a commit
that referenced
this pull request
Aug 19, 2026
…ull mimicry Findings from an adversarial review of this PR. The theme is silent behaviour changes versus the code this PR replaced. train_with_dpsgd now fails closed: a missing or misspelled "noise_multiplier" raised nothing and trained a fully non-private model from a function whose only purpose is DP-SGD. The key is required; non-private runs pass an explicit 0. make_opacus_compatible runs on both branches, not just the private one. Rewriting BatchNorm to GroupNorm only for private configs left the noise=0 anchor a structurally different model, so its utility gap mixed the cost of DP noise with the cost of an architecture change — the frontier's utility axis was not comparable end to end. Documented that the submitted architecture is therefore not necessarily what trains. Full mimicry is enforced instead of silently downgraded: n_ref_train used min(target, ref_pool), so a short reference pool quietly trained weaker references and biased the audit optimistic. A short pool is now an error, which is what the old per-example code did loudly. _patch_residual_blocks only rewrites blocks that still use the stock forward. Subclasses with their own forward (SE, attention) were silently replaced by the vanilla residual path — changing the model rather than making it Opacus-safe. Such blocks are now left alone with a warning. The "auc" utility metric refuses multiclass output instead of reshape(-1)-ing it into a meaningless number. The accountant travels with the epsilon in campaign_extras. The prv default is deliberate (it matches the webapp's DP-SGD path), but PRV and RDP epsilons are not comparable — measured 4.22 vs 4.87 for identical noise here — so a record that does not name its accountant cannot safely be compared with another run's. Examples: GRU-D regains the max_physical_batch=128 cap the pre-PR code set deliberately (it is the memory-heaviest target in the repo, and the shared 256 default would OOM mid-campaign), and gains the small-dataset split guard its LR sibling already had. The LOS campaign imports target_models.LR instead of redeclaring an identical LRSigmoid, so the campaign trains the same class the audited target used. One earlier test asserted the now-incorrect behaviour (BatchNorm surviving the non-private path) and has been inverted to assert architecture consistency. 47 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
fazelehh
added a commit
that referenced
this pull request
Aug 19, 2026
The webapp's DP-SGD loop carried a near-verbatim copy of the BatchNorm fix, in-place-activation pass and residual-forward patch that now live in leakpro.optimization.make_opacus_compatible. Sharing one implementation means a model trained through the wizard and the same model trained by a PET campaign are the same architecture, and the subclass-safety fix from the #447 review applies to both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
Collaborator
Author
|
Addressed the code-review findings on this PR. The theme was silent behaviour changes versus the per-example code this PR replaced. Correctness
Examples
Also: one of my earlier tests asserted the now-incorrect behaviour (BatchNorm surviving the non-private path) and has been inverted to assert architecture consistency. The webapp's duplicate Opacus-compat block is removed in #448, which now calls this shared helper. 47 tests pass on this branch, ruff clean. |
fazelehh
added a commit
that referenced
this pull request
Aug 20, 2026
…ull mimicry Findings from an adversarial review of this PR. The theme is silent behaviour changes versus the code this PR replaced. train_with_dpsgd now fails closed: a missing or misspelled "noise_multiplier" raised nothing and trained a fully non-private model from a function whose only purpose is DP-SGD. The key is required; non-private runs pass an explicit 0. make_opacus_compatible runs on both branches, not just the private one. Rewriting BatchNorm to GroupNorm only for private configs left the noise=0 anchor a structurally different model, so its utility gap mixed the cost of DP noise with the cost of an architecture change — the frontier's utility axis was not comparable end to end. Documented that the submitted architecture is therefore not necessarily what trains. Full mimicry is enforced instead of silently downgraded: n_ref_train used min(target, ref_pool), so a short reference pool quietly trained weaker references and biased the audit optimistic. A short pool is now an error, which is what the old per-example code did loudly. _patch_residual_blocks only rewrites blocks that still use the stock forward. Subclasses with their own forward (SE, attention) were silently replaced by the vanilla residual path — changing the model rather than making it Opacus-safe. Such blocks are now left alone with a warning. The "auc" utility metric refuses multiclass output instead of reshape(-1)-ing it into a meaningless number. The accountant travels with the epsilon in campaign_extras. The prv default is deliberate (it matches the webapp's DP-SGD path), but PRV and RDP epsilons are not comparable — measured 4.22 vs 4.87 for identical noise here — so a record that does not name its accountant cannot safely be compared with another run's. Examples: GRU-D regains the max_physical_batch=128 cap the pre-PR code set deliberately (it is the memory-heaviest target in the repo, and the shared 256 default would OOM mid-campaign), and gains the small-dataset split guard its LR sibling already had. The LOS campaign imports target_models.LR instead of redeclaring an identical LRSigmoid, so the campaign trains the same class the audited target used. One earlier test asserted the now-incorrect behaviour (BatchNorm surviving the non-private path) and has been inverted to assert architecture consistency. 47 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
fazelehh
force-pushed
the
feature/pet-recipe
branch
from
August 20, 2026 07:20
18db5a2 to
6d20b70
Compare
fazelehh
added a commit
that referenced
this pull request
Aug 20, 2026
The webapp's DP-SGD loop carried a near-verbatim copy of the BatchNorm fix, in-place-activation pass and residual-forward patch that now live in leakpro.optimization.make_opacus_compatible. Sharing one implementation means a model trained through the wizard and the same model trained by a PET campaign are the same architecture, and the subclass-safety fix from the #447 review applies to both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
fazelehh
added a commit
that referenced
this pull request
Aug 20, 2026
…ull mimicry Findings from an adversarial review of this PR. The theme is silent behaviour changes versus the code this PR replaced. train_with_dpsgd now fails closed: a missing or misspelled "noise_multiplier" raised nothing and trained a fully non-private model from a function whose only purpose is DP-SGD. The key is required; non-private runs pass an explicit 0. make_opacus_compatible runs on both branches, not just the private one. Rewriting BatchNorm to GroupNorm only for private configs left the noise=0 anchor a structurally different model, so its utility gap mixed the cost of DP noise with the cost of an architecture change — the frontier's utility axis was not comparable end to end. Documented that the submitted architecture is therefore not necessarily what trains. Full mimicry is enforced instead of silently downgraded: n_ref_train used min(target, ref_pool), so a short reference pool quietly trained weaker references and biased the audit optimistic. A short pool is now an error, which is what the old per-example code did loudly. _patch_residual_blocks only rewrites blocks that still use the stock forward. Subclasses with their own forward (SE, attention) were silently replaced by the vanilla residual path — changing the model rather than making it Opacus-safe. Such blocks are now left alone with a warning. The "auc" utility metric refuses multiclass output instead of reshape(-1)-ing it into a meaningless number. The accountant travels with the epsilon in campaign_extras. The prv default is deliberate (it matches the webapp's DP-SGD path), but PRV and RDP epsilons are not comparable — measured 4.22 vs 4.87 for identical noise here — so a record that does not name its accountant cannot safely be compared with another run's. Examples: GRU-D regains the max_physical_batch=128 cap the pre-PR code set deliberately (it is the memory-heaviest target in the repo, and the shared 256 default would OOM mid-campaign), and gains the small-dataset split guard its LR sibling already had. The LOS campaign imports target_models.LR instead of redeclaring an identical LRSigmoid, so the campaign trains the same class the audited target used. One earlier test asserted the now-incorrect behaviour (BatchNorm surviving the non-private path) and has been inverted to assert architecture consistency. 47 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
fazelehh
force-pushed
the
feature/pet-recipe
branch
from
August 20, 2026 07:58
6d20b70 to
c72393a
Compare
fazelehh
added a commit
that referenced
this pull request
Aug 20, 2026
The webapp's DP-SGD loop carried a near-verbatim copy of the BatchNorm fix, in-place-activation pass and residual-forward patch that now live in leakpro.optimization.make_opacus_compatible. Sharing one implementation means a model trained through the wizard and the same model trained by a PET campaign are the same architecture, and the subclass-safety fix from the #447 review applies to both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h
Adds leakpro/optimization/training.py with two entry points over a single Opacus loop, so every PET optimization run — library, example, webapp — trains through the same code: - fit_dpsgd(model, loader, criterion, optimizer, epochs, noise_multiplier, max_grad_norm, ...) has the shape of AbstractInputHandler.train on purpose. A handler forwards its arguments here and the RMIA shadow models are then trained by exactly the code that trained the target, which is what makes them mimic it. - train_with_dpsgd(recipe, config, indices, ...) builds model, loader and optimizer from a PETRecipe (factories) and calls fit_dpsgd. It fails closed: a missing noise_multiplier or max_grad_norm raises instead of silently training a non-private model. Every model, private or not, first goes through make_opacus_compatible (BatchNorm -> GroupNorm, in-place activations off, torchvision residual adds out of place) so all points on one frontier share an architecture. Because ModuleValidator.fix swaps children in place, the root module keeps its identity while its parameter set changes; fit_dpsgd therefore compares parameter identity, not object identity, and rebuilds the optimizer when they differ — an optimizer built before the rewrite would otherwise keep stepping detached BatchNorm parameters and never touch the GroupNorm ones. The accountant defaults to PRV and travels with epsilon as model.dp_accounting: PRV and RDP epsilons for the same noise are not comparable, so a number that does not name its accountant cannot be compared across runs. Supersedes the Campaign-based recipe branch: build_campaign_fns, the confidence-signal attack and the campaign example ports are gone, since the optimization layer now audits with the real RMIA attack via run_rmia_audit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RivRrjHEc3puv5EYAHLkAs
…t_dpsgd dp_handler.dp_train no longer carries its own PrivacyEngine/BatchMemoryManager loop; it reads the DP knobs from dpsgd_dic.pkl and forwards to leakpro.optimization.fit_dpsgd, so target and shadow models in the CIFAR example are trained by the same code as every other PET run. Training-set accuracy/loss are measured after the last epoch on the logical loader via a shared evaluate(), since the Poisson-sampled private loader does not visit every example exactly once. PrivacyUtilityConfig gains `accountant` (prv | rdp | gdp, default prv). The run writes it into each trial's dpsgd_dic.pkl and records it next to epsilon in the trial extras, so a frontier never reports an epsilon without naming the accountant that produced it. Default moves from the handler's hard-coded "rdp" to "prv" — the same noise now reads as a smaller epsilon, and the recorded accountant makes that visible rather than silent. Verified: `run_optimization.py --smoke` completes end to end (target, 3 RMIA shadow models, 2 Optuna trials) through the shared loop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RivRrjHEc3puv5EYAHLkAs
fazelehh
force-pushed
the
feature/pet-recipe
branch
from
September 21, 2026 12:33
c72393a to
aba8a5f
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
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.
Stacked on #438 (base is
feature/pet-optimization; GitHub retargets this to main automatically once #438 merges).What
Adds
leakpro/optimization/training.py: the structured training contract (PETRecipe) and the single shared Opacus path (train_with_dpsgd,build_campaign_fns) that replaces the DP-SGD loop previously duplicated in every campaign example. All three examples (LOS-LR, CIFAR, LOS GRU-D) are ported onto it and shrink to declaring a recipe; the standalone GRU-D script is superseded by the recipe-ported one.A
PETRecipeis the plan's structured contract: instead of a monolithictrain(data) -> model, the user supplies factories the optimizer can intervene in (make_model,make_optimizer,make_loader, criterion, epochs). Each factory receives the sampled knob config and reads what it needs, which is what makes the next PET cheap — a regularization bundle is justmake_modelreadingdropoutandmake_optimizerreadingweight_decay, with no interface change. Exotic loops that do not fit keep the escape hatch of writing the three campaign callables directly.Hardening beyond the refactor
make_opacus_compatible()— ModuleValidator pass (BatchNorm → GroupNorm), disables in-place activations, and rewrites torchvision residual blocks whose in-placeout += identityModuleValidator cannot see because it lives inforwardrather than a submodule. Without this, any BatchNorm architecture — including both webapp image presets — fails atmake_private()."prv"(was hard-coded"rdp"). RDP is the looser bound, so identical noise reported a larger ε here than via the webapp's DP-SGD path. This changes reported ε for existing campaigns (downward, for identical noise); empirical attack numbers are unaffected.argmax—argmaxover one column is always 0, silently yielding 0% or 100%.Review fixes applied
An adversarial review of this PR produced 10 findings; all are addressed in
18db5a2a. The theme was silent behaviour changes versus the per-example code this PR replaced.train_with_dpsgdfails closed. A missing or misspellednoise_multipliersilently trained a fully non-private model, from a function whose only purpose is DP-SGD. The key is now required; non-private runs pass an explicit0.0.make_opacus_compatibleruns on both branches. Rewriting BatchNorm → GroupNorm only for private configs left the noise=0 anchor a structurally different model, so its utility gap mixed the cost of DP noise with the cost of an architecture change — the frontier's utility axis was not comparable end to end. Documented consequence: the submitted architecture is not necessarily what trains.n_ref_trainusedmin(target, ref_pool), so a short reference pool quietly trained weaker references and biased the audit optimistic. A short pool is now an error — which is what the old per-example code did loudly._patch_residual_blocksonly rewrites blocks still using the stockforward. Subclasses with their own forward (SE, attention) were being silently replaced by the vanilla residual path, changing the model rather than making it Opacus-safe. Such blocks are now left alone with a warning.aucutility metric refuses multiclass output instead ofreshape(-1)-ing it into a meaningless number.campaign_extras. Theprvdefault is deliberate, but PRV and RDP epsilons are not comparable — measured 4.22 vs 4.87 for identical noise — so a record that does not name its accountant cannot safely be compared with another run's.Examples: GRU-D regains the
max_physical_batch=128cap the pre-PR code set deliberately (it is the memory-heaviest target in the repo; the shared 256 default would OOM mid-campaign) and gains the small-dataset split guard its LR sibling already had. The LOS campaign importstarget_models.LRinstead of redeclaring an identicalLRSigmoid, so it trains the same class the audited target used.One earlier test asserted the now-incorrect behaviour (BatchNorm surviving the non-private path) and has been inverted to assert architecture consistency instead.
Verification
pytest leakpro/tests/test_optimization/— 47 tests pass.ruff check .clean.The webapp's near-verbatim copy of the Opacus-compat block is removed in #448, which now calls this shared helper.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QBPG7MprEqhqG95fFURk8h