mirror of
https://github.com/wiltodelta/remove-ai-watermarks.git
synced 2026-08-09 23:50:40 +02:00
Fix what the verification pass found in the knob removal
An adversarial review of52b2c11(five independent audits, each finding put to two skeptics, plus a completeness critic) found four defects that commit introduced and several stale claims it should have caught. The install hint no longer installs -- again. Folding five hints into one INVISIBLE_EXTRA constant dropped the shell quoting the originals had, so the printed remediation was `pip install remove-ai-watermarks[qwen-zimage]`. Bare brackets are a glob in zsh, the macOS default shell: it dies with "no matches found" before pip runs. That is the exact failure52b2c11existed to stop producing, reintroduced in a different form by a bulk replace. The constant is quoted now, and a test asserts the quotes rather than the bare substring -- the old assertions passed either way, which is why nothing caught it. Three tests were not guarding what they claimed: - The commit's headline behaviour change, per-profile polish resolution inside the engine, had no test at all. Rebinding resolve_adaptive_polish to the pre-commit `bool(value)` left the full suite green. Now covered by a test that drives the real engine and observes whether humanizer.adaptive_polish ran; that mutation now fails it. - TestAvailability still asserted the pre-commit (torch, diffusers) contract, so in a diffusion-only environment it was simply wrong, and comparing each gate to a tuple copied from itself could never catch the two gates disagreeing -- the drift the shared REMOVAL_MODULES was introduced to prevent. Replaced with a test that simulates each module's absence and requires BOTH gates to close. - Both CUDA-refusal guards skipped in every environment, including CI: they were gated on the diffusion stack, which no CI job installs. The refusal fires before any torch attribute is read, so they now run everywhere; only the dtype assertion keeps its skip. Also: the retired-knob test covered `invisible` but not `all` or `batch`, though all three declared those options separately; and smoke_matrix.py still called remove_watermark(region=...), a parameter52b2c11deleted, with the resulting TypeError swallowed into a skip by a broad except. Stale documentation the previous sweep missed: known-limitations still described an MPS out-of-memory fallback and a lighter-pipeline escape that no code can produce; module-internals declared Canny thresholds of 100/200 as a compatibility contract while the code uses 13/64, attributed enable_model_cpu_offload to deleted profiles, and still warned that the engine and CLI defaults differ (this commit's predecessor made them identical); cli.md gated `all` on the `diffusion` extra; python-api claimed "cuda" was the only accepted explicit device when "auto" is too. The claim that `device` is not a parameter was wrong in both module-internals and .claude/rules/development.md -- it is one, deliberately, and now says so. `--cpu-offload` help and the pipeline's CUDA guard both still pointed at MPS. Not fixed here, reported instead -- both are outside this repo: - ComfyUI-remove-ai-watermarks nodes.py:332 passes num_inference_steps and guidance_scale (plus min_resolution/upscaler frombf4bfc1). distribute.yml's comfyui job runs on every release and fails the release if the node sync fails, so 0.25.0 needs that node updated first. - raiw-app modal_app.py:422-425 forwards the same two kwargs into remove_watermark. Latent: it is pinned to1a77e24and nothing supplies a value today, so it fires on the next pin bump. pre-commit: 1) maintain.sh - exit 0 (1093 tests, Pyright 0 errors, no vulnerabilities); 2) /simplify - not re-run, this commit is the applied output of a five-dimension adversarial review; 3) docs sync - grepped MPS/mps, the extras names and every symbol touched across README, docs/, scripts/, .claude/; updated 6 docs; 4) CLAUDE.md - corrected the device claim in .claude/rules/development.md and added the shell-quoting rule Verified by execution, not assertion: smoke_matrix --quick 51 pass / 0 fail, _knob_rows driven directly 10 pass / 0 fail / 7 skip (no CUDA), the install hint rendered and round-tripped through zsh, and each new test confirmed to fail under the mutation it is meant to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
52b2c115e8
commit
2f72257996
+1
-1
@@ -433,7 +433,7 @@ The command runs:
|
||||
|
||||
The visible options and diffusion options are also available on `all`.
|
||||
|
||||
If diffusion is required but the `diffusion` extra is unavailable, `all` still
|
||||
If invisible removal is required but the `qwen-zimage` extra is unavailable, `all` still
|
||||
writes the result of the visible and metadata stages, prints a prominent
|
||||
warning, and exits with code 1. This prevents a partial result from being
|
||||
reported as complete.
|
||||
|
||||
+14
-12
@@ -179,23 +179,25 @@ overlapping tiles. It avoids the explicit downscale, but it is not pixel
|
||||
lossless: each tile is independently regenerated. With `qwen-zimage`, only the
|
||||
global Qwen stage is tiled; the face stage runs after tile blending.
|
||||
|
||||
### CPU offload is CUDA only
|
||||
### CPU offload trades speed for VRAM
|
||||
|
||||
`--cpu-offload` moves Diffusers model components between CPU and CUDA instead of
|
||||
keeping the complete standard pipeline in GPU memory. For `qwen-zimage`, it
|
||||
forces the face stack to use its offload path.
|
||||
`--cpu-offload` forces both stacks of the two-stage profile out of automatic
|
||||
device residency, streaming weights instead of pinning them. It reduces CUDA
|
||||
memory pressure at the cost of speed.
|
||||
|
||||
The option reduces CUDA memory pressure at the cost of speed. It has no effect
|
||||
on CPU or MPS and fails loudly when a CUDA Diffusers pipeline does not expose
|
||||
the required offload method.
|
||||
Residency is otherwise chosen from the card's total VRAM. On a card large enough
|
||||
to hold a stack, offloading is pure waste: DiffSynth drops weights to the meta
|
||||
device and re-reads every parameter from disk on each stage transition.
|
||||
|
||||
### MPS may fall back to CPU
|
||||
### There is no CPU or MPS fallback
|
||||
|
||||
The SDXL paths include an MPS out-of-memory fallback that reloads on CPU. A run
|
||||
that appears much slower after an MPS failure may be continuing on CPU.
|
||||
Invisible-watermark removal refuses any device but CUDA at construction. There is
|
||||
no MPS out-of-memory fallback that continues on CPU, and no lighter profile to
|
||||
drop to: both remaining profiles need an NVIDIA GPU.
|
||||
|
||||
Memory needs depend on the pipeline, input size, dtype, and machine. Use tiling,
|
||||
a resolution cap, or a lighter pipeline when necessary.
|
||||
When a run does not fit, the levers are `--tile` (native geometry, tiled
|
||||
diffusion), `--max-resolution` (an explicit downscale), and `--cpu-offload`.
|
||||
Memory needs depend on the profile, input size, dtype, and card.
|
||||
|
||||
## Metadata and formats
|
||||
|
||||
|
||||
+32
-21
@@ -502,15 +502,21 @@ mandatory global stage and YuNet while leaving the optional Z-Image and SAM face
|
||||
stack lazy until a face is detected. The default `preload()` still loads every
|
||||
stage.
|
||||
|
||||
**What is deliberately not a parameter.** Model id, step count, CFG and any
|
||||
non-CUDA device are fixed by the profile, so none of them appears in
|
||||
`WatermarkRemover.__init__`, `remove_watermark`, `InvisibleEngine`, or the CLI.
|
||||
They used to be accepted and then rejected several frames down; a signature that
|
||||
refuses the argument outright fails where the caller can act on it, and stops a
|
||||
wrapper from threading a value that would silently do nothing. The step count and
|
||||
CFG live with the stage that runs them (`GLOBAL_STEPS`, `FACE_STEPS`, `GLOBAL_CFG`,
|
||||
`FACE_CFG` in `qwen_zimage_pipeline.py`). The dtype is likewise profile-owned: see
|
||||
"Face-stage dtype" for what an override cost the last time one existed.
|
||||
**What is deliberately not a parameter.** Model id, step count and CFG are fixed
|
||||
by the profile, so none of them appears in `WatermarkRemover.__init__`,
|
||||
`remove_watermark`, `InvisibleEngine`, or the CLI. They used to be accepted and
|
||||
then rejected several frames down; a signature that refuses the argument outright
|
||||
fails where the caller can act on it, and stops a wrapper from threading a value
|
||||
that would silently do nothing. The step count and CFG live with the stage that
|
||||
runs them (`GLOBAL_STEPS`, `FACE_STEPS`, `GLOBAL_CFG`, `FACE_CFG` in
|
||||
`qwen_zimage_pipeline.py`). The dtype is likewise profile-owned: see "Face-stage
|
||||
dtype" for what an override cost the last time one existed.
|
||||
|
||||
`device` is the exception and remains a library parameter: `None` or `"auto"`
|
||||
detect, `"cuda"` pins without detecting (which is what a container that knows its
|
||||
hardware wants), and any other value raises at construction. It is not a CLI
|
||||
option, because the only useful value a user could type is the one detection
|
||||
already returns.
|
||||
|
||||
[`invisible_engine.py`](../src/remove_ai_watermarks/invisible_engine.py) handles
|
||||
image sizing, postprocessing, and the public engine
|
||||
@@ -523,16 +529,20 @@ and reporting it implied an Apple-silicon or Intel-GPU path that does not exist.
|
||||
The refusal names the *resolved* device, so `device=None` on a CUDA-less host says
|
||||
`'cpu'` rather than `'None'`.
|
||||
|
||||
The Python engine and CLI do not have identical defaults for every optional
|
||||
postprocessing argument. Integrations that require reproducibility should pass
|
||||
the relevant values explicitly.
|
||||
The Python engine and the CLI now resolve the same defaults: the CLI forwards an
|
||||
unset `--adaptive-polish` and `--seed` as `None` and the engine applies the
|
||||
profile's answer, so a library caller and a CLI caller on one profile produce the
|
||||
same pixels. They diverged before, in opposite directions, for exactly this knob.
|
||||
|
||||
The standard Qwen and ControlNet prompts are calibrated model inputs, and the
|
||||
ControlNet edge map uses fixed Canny thresholds of 100 and 200. Treat those
|
||||
values as behavioral compatibility contracts: a refactor must preserve them,
|
||||
and any deliberate change requires image-quality evaluation rather than only a
|
||||
unit-test pass. Exact prompt and edge-map regression guards live in
|
||||
`test_platform.py` and `test_invisible_engine.py`.
|
||||
The global and face prompts are calibrated model inputs, and the Canny edge map
|
||||
uses fixed thresholds of `_CANNY_LOW = 13` / `_CANNY_HIGH = 64`
|
||||
(`qwen_zimage_pipeline.py`). Treat those values as behavioral compatibility
|
||||
contracts: a refactor must preserve them, and any deliberate change requires
|
||||
image-quality evaluation rather than only a unit-test pass. The prompt and
|
||||
edge-map regression guards are
|
||||
`test_qwen_zimage_pipeline.py::test_global_kwargs_use_lightning_and_diffsynth_controlnet_shape`,
|
||||
`::test_face_kwargs_use_project_zimage_settings` and
|
||||
`::test_canny_control_image_is_three_channel_and_detects_an_edge`.
|
||||
|
||||
Regression coverage:
|
||||
|
||||
@@ -542,9 +552,10 @@ Regression coverage:
|
||||
|
||||
### CPU offload
|
||||
|
||||
CPU offload is enabled only when requested on CUDA. The standard Diffusers
|
||||
profiles call `enable_model_cpu_offload`. The `qwen-zimage` profile uses the
|
||||
same flag to force **both** its stacks out of automatic device residency.
|
||||
CPU offload is enabled only when requested. Nothing calls Diffusers'
|
||||
`enable_model_cpu_offload` any more -- that belonged to the deleted single-stage
|
||||
profiles. `--cpu-offload` now forces **both** stacks of the two-stage profiles out
|
||||
of automatic device residency.
|
||||
|
||||
Residency is otherwise chosen from the card's total VRAM, once per stack:
|
||||
`resolve_global_model_residency` gates the mandatory Qwen stack at
|
||||
|
||||
+3
-3
@@ -401,9 +401,9 @@ engine.remove_watermark(
|
||||
)
|
||||
```
|
||||
|
||||
`device=None` detects CUDA. The only other accepted value is `"cuda"`; anything
|
||||
else raises at construction rather than deferring a guaranteed failure to model
|
||||
load time.
|
||||
`device=None` and `device="auto"` both run detection. `"cuda"` pins it without
|
||||
detecting. Every other value raises at construction rather than deferring a
|
||||
guaranteed failure to model-load time.
|
||||
|
||||
For limited CUDA memory:
|
||||
|
||||
|
||||
@@ -86,8 +86,10 @@ sweep asserts on stderr, or the codes get split -- the latter is the better fix.
|
||||
#### Coverage method
|
||||
|
||||
Compare the flags and values exercised by the matrix against the options declared by
|
||||
the CLI. Include optional backends, batch modes, tiling, region-targeted composition,
|
||||
and the ffmpeg audio/video strip. The gap to find is not only
|
||||
the CLI. Include optional backends, batch modes, tiling, and the ffmpeg audio/video
|
||||
strip. (Region-targeted diffusion composition was on this list until the parameter
|
||||
was deleted: nothing but a caller-less convenience wrapper ever reached it, and the
|
||||
user-facing region path is `erase`, which inpaints instead.) The gap to find is not only
|
||||
"logic untested" but
|
||||
"never executed on real data", which is precisely what this campaign is for.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user