diff --git a/.github/workflows/pr-review-status-labels.yml b/.github/workflows/pr-review-status-labels.yml index 4777ad21e..dd797f7e1 100644 --- a/.github/workflows/pr-review-status-labels.yml +++ b/.github/workflows/pr-review-status-labels.yml @@ -15,8 +15,15 @@ permissions: pull-requests: write issues: write +# Per-PR concurrency: a single shared group silently drops the earlier PR's sync +# when reviews land on two PRs within seconds (GitHub keeps one pending run per +# group), while no group at all lets same-PR runs race each other's label writes. +# Same-repo PRs serialize by PR number. Fork PRs (workflow_run.pull_requests is +# empty for them) serialize by head repo+branch — equivalent per PR in practice +# (two open PRs off one head branch would share a group). Manual dispatch sweeps +# share their own literal fallback group. concurrency: - group: pr-review-status-labels + group: pr-review-status-labels-${{ github.event.workflow_run.pull_requests[0].number || format('{0}-{1}', github.event.workflow_run.head_repository.full_name, github.event.workflow_run.head_branch) }} cancel-in-progress: false jobs: @@ -75,9 +82,14 @@ jobs: [ "$label" = "$target" ] && continue if jq -e --arg name "$label" '.labels | any(.name == $name)' <<<"$pr" >/dev/null; then encoded=$(jq -rn --arg label "$label" '$label | @uri') - gh api --method DELETE \ - "repos/$GITHUB_REPOSITORY/issues/$number/labels/$encoded" \ - >/dev/null + # A racing run may have removed the label already, so a 404 is benign — + # but ONLY a 404. Swallowing 403/5xx would turn permission regressions + # into green runs with wrong labels (the one failure ever observed in + # production was exactly a 403). + if ! out=$(gh api --method DELETE \ + "repos/$GITHUB_REPOSITORY/issues/$number/labels/$encoded" 2>&1 >/dev/null); then + grep -q "HTTP 404" <<<"$out" || { echo "$out" >&2; return 1; } + fi fi done diff --git a/.github/workflows/pr-status-labels.yml b/.github/workflows/pr-status-labels.yml index 38ffe0c4a..c6b6302fc 100644 --- a/.github/workflows/pr-status-labels.yml +++ b/.github/workflows/pr-status-labels.yml @@ -2,7 +2,7 @@ name: PR Status Labels on: pull_request_target: - types: [opened, reopened, ready_for_review, converted_to_draft, synchronize] + types: [opened, reopened, ready_for_review, converted_to_draft, synchronize, closed] permissions: # Labeling a PR goes through the issues API endpoint, but GitHub scopes the @@ -30,13 +30,36 @@ jobs: jq -e --arg name "$1" '.pull_request.labels | any(.name == $name)' "$event" >/dev/null } - [ "$(jq -r '.pull_request.state' "$event")" = "open" ] || exit 0 + if [ "$(jq -r '.pull_request.state' "$event")" != "open" ]; then + # Merged/closed PRs shed their status label so board views stay clean. + # Delete all four unconditionally: the payload's label snapshot can miss a + # label that a review-sync run racing the close instant wrote afterwards, + # and no later event ever repairs a closed PR (the dispatch sweep is + # open-only). A 404 (label absent) is benign; anything else fails loudly. + for label in "wip" "need review" "changes requested" "approved"; do + encoded=$(jq -rn --arg l "$label" '$l | @uri') + if ! out=$(gh api --method DELETE \ + "repos/$GITHUB_REPOSITORY/issues/$number/labels/$encoded" 2>&1 >/dev/null); then + echo "$out" | grep -q "HTTP 404" || { echo "$out" >&2; exit 1; } + fi + done + exit 0 + fi if [ "$draft" = "true" ]; then target="wip" elif [ "$(jq -r '.action' "$event")" = "synchronize" ]; then - # New commits only move the status back to review after changes were requested. - has_label "changes requested" || exit 0 - target="need review" + # New commits move the status back to review after changes were requested, + # and self-heal a PR that carries no status label at all (missed event or + # transient write failure) — otherwise keep whatever label is there. + # Scope is deliberately zero-label-only: a stale-but-present label + # self-corrects on the next review event or dispatch sweep. + if has_label "changes requested"; then + target="need review" + elif ! has_label "wip" && ! has_label "need review" && ! has_label "approved"; then + target="need review" + else + exit 0 + fi else target="need review" fi diff --git a/.gitignore b/.gitignore index cbd8ceeff..baa3010da 100644 --- a/.gitignore +++ b/.gitignore @@ -72,10 +72,15 @@ local/ *.pkl *.log -# Datasets: keep the committed pickscore prompts, ignore other (local-only) datasets +# Datasets: keep committed prompt sets and converter code, ignore local-only data datasets/* !datasets/pickscore !datasets/geneval2 +!datasets/video_r1_260k +!datasets/daily_omni_av +# converter outputs stay local-only; only the converter code is tracked +datasets/video_r1_260k/**/*.jsonl +datasets/daily_omni_av/**/*.jsonl # Large model artifacts (defense-in-depth) models/**/*.bin @@ -105,5 +110,6 @@ benchmarks/text/aime/data/aime2025.jsonl benchmarks/text/gpqa/data/ benchmarks_results/ -# Private experimental packages stay local-only (the tier is also not packaged). -experimental/private_*/ +# Private experimental packages (dirs or stray files) stay local-only +# (the tier is also not packaged). +experimental/private_* diff --git a/examples/diffusion/bagel/bagel_trainside_lora.yaml b/examples/diffusion/bagel/bagel_trainside_lora.yaml index 61a72b3d8..6e21701dd 100644 --- a/examples/diffusion/bagel/bagel_trainside_lora.yaml +++ b/examples/diffusion/bagel/bagel_trainside_lora.yaml @@ -16,7 +16,7 @@ # the diffuse↔replay ratio==1 bit-exactness. This = the known-good v3 config on the # central-kitchen (reuse) Bagel stack. # -# Launch: bash scripts/migration/run_reuse_smoke.sh (offline smoke) or a long-run launcher +# Launch: bash examples/run_experiment_single_node.sh diffusion/bagel/bagel_trainside_lora # Compose: cd UniRL-main && PYTHONPATH=$PWD python -m unirl.train_diffusion \ # --config-name diffusion/bagel/bagel_trainside_lora --cfg job --resolve diff --git a/examples/diffusion/qwen_image/qwen_image_sglang.yaml b/examples/diffusion/qwen_image/qwen_image_sglang.yaml index 93a9be1d4..e4910b5a4 100644 --- a/examples/diffusion/qwen_image/qwen_image_sglang.yaml +++ b/examples/diffusion/qwen_image/qwen_image_sglang.yaml @@ -21,7 +21,7 @@ # (single-space negative — empty "" degenerates after the 34-token chat-template # strip and divides the norm-preserving CFG blend by ~0; see QwenImagePipeline.) # -# 1x8: bash scripts/run_experiment_single_node.sh diffusion_rl/qwen_image_sglang +# 1x8: bash examples/run_experiment_single_node.sh diffusion/qwen_image/qwen_image_sglang num_devices: 8 batch_size: 48 # prompts_per_rollout (matches qwen_image_dancegrpo) diff --git a/experimental/README.md b/experimental/README.md index 86fae5f76..b9c6cf78a 100644 --- a/experimental/README.md +++ b/experimental/README.md @@ -54,6 +54,13 @@ graduates into core instead of being borrowed sideways. 6. **Model/engine variation goes through `_target_` polymorphism** — an `if model_family == ...` branch in package code is a review reject. +Rules 1, 2, and 4 are script-enforced (`lint/check_experimental_boundaries.py`; +`lint/check_recipe_targets.py`, which also rejects recipes outside this tier +targeting `experimental.*` and recipes targeting a sibling package). Rules 3, +5, and 6 are enforced in review. The boundary hook scans the working tree, so +uncommitted `private_*` packages are lint-gated locally — their only gate, as +CI never sees them. + ## Graduation Both directions are deliberate PRs, never drive-bys. Up: a second diff --git a/experimental/refl/models/sd3.py b/experimental/refl/models/sd3.py index 24c4f032c..2599096fb 100644 --- a/experimental/refl/models/sd3.py +++ b/experimental/refl/models/sd3.py @@ -99,12 +99,17 @@ def diffuse_with_grad( ) latents = initial_latents.to(device=device, dtype=self.trajectory_dtype) else: + if params.seed is None: + raise ValueError( + "REFL's fixed-noise regime needs an explicit sampling seed " + "(roles.py: params.seed is used verbatim every rollout/rank); set sampling.seed." + ) latents = self.generate_latents( batch_size=batch_size, latent_shape=latent_shape, device=device, dtype=self.trajectory_dtype, - base_seed=int(params.seed) if params.seed is not None else None, + base_seed=int(params.seed), ) sk: Dict[str, Any] = dict(params.sampler_kwargs or {}) diff --git a/experimental/refl/models/wan21.py b/experimental/refl/models/wan21.py index 508289c67..49cb13f00 100644 --- a/experimental/refl/models/wan21.py +++ b/experimental/refl/models/wan21.py @@ -218,6 +218,11 @@ def diffuse_with_grad( ) latents = initial_latents.to(device=device, dtype=self.trajectory_dtype) else: + if params.seed is None: + raise ValueError( + "REFL's fixed-noise regime needs an explicit sampling seed " + "(roles.py: params.seed is used verbatim every rollout/rank); set sampling.seed." + ) latents = self.generate_latents( batch_size=batch_size, latent_shape=latent_shape, diff --git a/experimental/refl/models/wan22.py b/experimental/refl/models/wan22.py index 74d865d1a..415527bbf 100644 --- a/experimental/refl/models/wan22.py +++ b/experimental/refl/models/wan22.py @@ -186,6 +186,11 @@ def diffuse_with_grad( ) latents = initial_latents.to(device=device, dtype=self.trajectory_dtype) else: + if params.seed is None: + raise ValueError( + "REFL's fixed-noise regime needs an explicit sampling seed " + "(roles.py: params.seed is used verbatim every rollout/rank); set sampling.seed." + ) latents = self.generate_latents( batch_size=batch_size, latent_shape=latent_shape, diff --git a/experimental/refl/reward/face/requirements.txt b/experimental/refl/reward/face/requirements.txt index 329432f90..bc560ab33 100644 --- a/experimental/refl/reward/face/requirements.txt +++ b/experimental/refl/reward/face/requirements.txt @@ -1,3 +1,6 @@ +# Face reward extras — ADDITIVE ONLY on top of the UniRL core stack. +# torch / torchvision (used by face_tools.py) come from the core engine extra +# (pyproject.toml); additive-only forbids re-declaring them here. imageio>=2.31 imageio-ffmpeg>=0.4 onnx>=1.14 diff --git a/experimental/refl/reward/videoalign/requirements.txt b/experimental/refl/reward/videoalign/requirements.txt index bcd1c63c0..502114e8a 100644 --- a/experimental/refl/reward/videoalign/requirements.txt +++ b/experimental/refl/reward/videoalign/requirements.txt @@ -6,4 +6,7 @@ # transformers / peft / safetensors / huggingface-hub / einops (see # pyproject.toml); never re-pin those here. Attention runs SDPA — flash-attn # is not part of the locked stack. -torchvision # frame preprocessing transforms; matches the engine extra's torch build +# +# torchvision (frame preprocessing) also comes from the core engine extra — +# additive-only forbids re-declaring it, and a bare re-pin could resolve a +# cu-mismatched wheel that downgrades torch itself. Nothing extra to install. diff --git a/lint/check_experimental_boundaries.py b/lint/check_experimental_boundaries.py index 58a61b4eb..a9a0369a8 100755 --- a/lint/check_experimental_boundaries.py +++ b/lint/check_experimental_boundaries.py @@ -7,14 +7,18 @@ 1. **Core never imports experimental.** ``unirl/`` must not reference the ``experimental`` namespace — the dependency arrow points one way. 2. **Experimental packages never import each other.** ``experimental/`` - may import ``unirl`` and its own package only; sharing code across - packages means it is ready to graduate into core, not ready to be - borrowed sideways. + may import ``unirl`` and its own package only — in any spelling: + absolute, ``from experimental import ``, bare ``import experimental``, + or a relative import that climbs past the package root. Top-level + ``experimental/*.py`` files are not a shared space and may not import + the tier at all. 3. **Recipe requirements are additive-only.** Reward and actor share one - Python process, so an ``experimental/**/requirements.txt`` cannot - version-"isolate" anything: any requirement whose (normalized) name is - already declared in ``pyproject.toml`` core dependencies would mutate - the locked core stack on install and is rejected. + Python process, so an ``experimental/**/requirements*.txt`` cannot + version-"isolate" anything: any requirement whose (normalized) name the + locked core stack already governs — ``[project.dependencies]``, every + ``[project.optional-dependencies]`` extra, or a ``[tool.uv]`` override + pin — would mutate that stack on install and is rejected, as are pip + option/include lines and URL requirements the name parser cannot vet. Run by the ``check-experimental-boundaries`` pre-commit hook. Exits non-zero listing each violation. @@ -39,26 +43,44 @@ def _normalize(name: str) -> str: def _py_files(base: Path): for path in sorted(base.rglob("*.py")): - if not SKIP_PARTS.intersection(path.parts): + # Intersect repo-relative parts: an absolute-path match would let a checkout + # that merely lives under a directory named "vendor" skip every rule. + if not SKIP_PARTS.intersection(path.relative_to(ROOT).parts): yield path -def _imported_roots(path: Path): +def _imports(path: Path): + """Yield ``(module, from_names)`` per import statement in ``path``. + + ``import a.b`` → ``("a.b", ())``; ``from a import b, c`` → ``("a", ("b", "c"))``. + Relative imports are resolved against the file's package (its directory + chain under ROOT) before yielding; a level that climbs out of the tree + yields nothing. + """ try: tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) except (OSError, SyntaxError): return + pkg = path.relative_to(ROOT).parts[:-1] for node in ast.walk(tree): if isinstance(node, ast.Import): for alias in node.names: - yield alias.name - elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: - yield node.module + yield alias.name, () + elif isinstance(node, ast.ImportFrom): + names = tuple(alias.name for alias in node.names) + if node.level == 0: + if node.module: + yield node.module, names + elif node.level <= len(pkg): + anchor = pkg[: len(pkg) - (node.level - 1)] + parts = (*anchor, *(node.module.split(".") if node.module else ())) + if parts: + yield ".".join(parts), names def check_core_does_not_import_experimental(errors: list[str]) -> None: for path in _py_files(ROOT / "unirl"): - for module in _imported_roots(path): + for module, _ in _imports(path): if module == "experimental" or module.startswith("experimental."): errors.append(f"{path.relative_to(ROOT)}: core imports {module!r} — the arrow points the other way") @@ -70,34 +92,70 @@ def check_no_cross_package_imports(errors: list[str]) -> None: for path in _py_files(exp): rel = path.relative_to(exp) own = rel.parts[0] if len(rel.parts) > 1 else None - for module in _imported_roots(path): - if not module.startswith("experimental."): - continue - target = module.split(".")[1] if "." in module else None - if target and own and target != own: + + def flag(module: str) -> None: + if own is None: + errors.append( + f"{path.relative_to(ROOT)}: imports {module!r} from top-level experimental/ — " + "the tier root is not a shared space; code lives inside one package" + ) + else: errors.append( f"{path.relative_to(ROOT)}: imports {module!r} from a sibling package — " "shared code graduates into core, it is not borrowed sideways" ) + for module, names in _imports(path): + if module == "experimental": + if not names: + errors.append( + f"{path.relative_to(ROOT)}: bare 'import experimental' reaches every sibling — " + "import your own package or core explicitly" + ) + for name in names: + if name != own: + flag(f"experimental.{name}") + elif module.startswith("experimental."): + if module.split(".")[1] != own: + flag(module) + + +def _core_dependency_names(pyproject: dict) -> set[str]: + """Every name whose version the locked core stack already governs.""" + deps = list(pyproject["project"]["dependencies"]) + for extra in pyproject["project"].get("optional-dependencies", {}).values(): + deps.extend(extra) + deps.extend(pyproject.get("tool", {}).get("uv", {}).get("override-dependencies", [])) + return {_normalize(_REQ_NAME_RE.match(dep).group(1)) for dep in deps} + def check_requirements_additive_only(errors: list[str]) -> None: - pyproject = tomllib.loads((ROOT / "pyproject.toml").read_text(encoding="utf-8")) - core = {_normalize(_REQ_NAME_RE.match(dep).group(1)) for dep in pyproject["project"]["dependencies"]} + core = _core_dependency_names(tomllib.loads((ROOT / "pyproject.toml").read_text(encoding="utf-8"))) exp = ROOT / "experimental" if not exp.is_dir(): return - for req_file in sorted(exp.rglob("requirements.txt")): - if SKIP_PARTS.intersection(req_file.parts): + for req_file in sorted(exp.rglob("requirements*.txt")): + if SKIP_PARTS.intersection(req_file.relative_to(ROOT).parts): continue - for line in req_file.read_text(encoding="utf-8").splitlines(): - line = line.split("#", 1)[0].strip() + rel = req_file.relative_to(ROOT) + for lineno, raw in enumerate(req_file.read_text(encoding="utf-8").splitlines(), 1): + line = raw.split("#", 1)[0].strip() if not line: continue + if line.startswith("-"): + errors.append( + f"{rel}:{lineno}: {line!r} — pip option/include lines are not allowed; " + "declare additive name-based pins only" + ) + continue match = _REQ_NAME_RE.match(line) - if match and _normalize(match.group(1)) in core: + rest = line[match.end() :] if match else "" + # "@" covers PEP 508 direct references, spaced or not (name@git+https://...). + if not match or (rest and rest[0] not in " \t@[<>=!~;,"): + errors.append(f"{rel}:{lineno}: {line!r} — unparseable requirement; use PEP 508 name-based pins") + elif _normalize(match.group(1)) in core: errors.append( - f"{req_file.relative_to(ROOT)}: {line!r} re-declares a core dependency — " + f"{rel}:{lineno}: {line!r} re-declares a core dependency — " "requirements are additive-only (same-process colocation cannot version-isolate)" ) diff --git a/lint/check_recipe_targets.py b/lint/check_recipe_targets.py index ec1558656..2fb5e185a 100755 --- a/lint/check_recipe_targets.py +++ b/lint/check_recipe_targets.py @@ -131,16 +131,40 @@ def main() -> int: top_level = {path: _top_level_names(source, path) for path, source in zip(needed, _read_all(needed))} failures: list[str] = [] + tier: list[str] = [] for path, lineno, dotted in targets: hit = module_split[dotted] names = top_level[hit[0]] if hit is not None else None if names is None or hit[1] not in names: failures.append(f"{path.relative_to(ROOT)}:{lineno}: unresolved _target_ '{dotted}'") + if dotted.startswith("experimental."): + # Tier direction: only experimental's own recipes may wire experimental code, + # and only within their own package (mirrors check_experimental_boundaries). + rel = path.relative_to(ROOT) + if rel.parts[0] != "experimental": + tier.append( + f"{rel}:{lineno}: core recipe targets '{dotted}' — the dependency arrow points the other way" + ) + elif len(rel.parts) == 2: + tier.append( + f"{rel}:{lineno}: recipe at the experimental/ root targets '{dotted}' — " + "a recipe belongs inside one package (experimental//...)" + ) + elif dotted.split(".")[1] != rel.parts[1]: + tier.append( + f"{rel}:{lineno}: recipe under 'experimental/{rel.parts[1]}/' targets sibling '{dotted}' — " + "shared code graduates into core" + ) if failures: print("Unresolved recipe _target_ paths (rename leftover or typo):", file=sys.stderr) for f in failures: print(f" {f}", file=sys.stderr) + if tier: + print("Experimental-tier direction violations:", file=sys.stderr) + for f in tier: + print(f" {f}", file=sys.stderr) + if failures or tier: return 1 print(f"check-recipe-targets: {len(targets)} recipe _target_ paths resolve.") return 0 diff --git a/unirl/distributed/group/worker.py b/unirl/distributed/group/worker.py index 1949d8aa6..c2ecf8dfd 100644 --- a/unirl/distributed/group/worker.py +++ b/unirl/distributed/group/worker.py @@ -312,11 +312,6 @@ def resolve(o): # Cross-RPC autograd can only propagate gradients back to controller-side # TensorRef inputs recorded by Handle as input_metas. tensors = [fetched[str(i)] for i in range(len(in_metas))] - if len(tensors) != len(in_metas): - raise RuntimeError( - f"Worker.call grad input mismatch for {method_name} call_id={call_id}: " - f"saved {len(tensors)} tensors for {len(in_metas)} TensorRef inputs" - ) for t in tensors: t.requires_grad_(True) t.retain_grad() diff --git a/unirl/models/bagel/pipeline.py b/unirl/models/bagel/pipeline.py index 3444cdebb..524f2f0d5 100644 --- a/unirl/models/bagel/pipeline.py +++ b/unirl/models/bagel/pipeline.py @@ -288,12 +288,25 @@ def _build_contexts(self, prompt: str, image: Optional[Any] = None) -> Tuple[Any inf = self.bundle.inferencer gen = inf.init_gen_context() cfg_img = deepcopy(gen) - with torch.no_grad(), self._autocast_ctx(): - if image is not None: - gen = self._update_context_image(self._resize_input_image(image), gen, vae=True, vit=True) - cfg_text = deepcopy(gen) # snapshot before the prompt text → drop-text branch - gen = inf.update_context_text(prompt, gen) - cfg_img = inf.update_context_text(prompt, cfg_img) + # eval() is load-bearing (same contract as ``BagelDiffusionStage._build_contexts_from_prompt`` + # and ``rl_ops.forward_flow``): navit dispatches ``forward_train`` vs ``forward_inference`` + # on ``self.training`` and these prefills use the packed-query inference signature — in + # train() they TypeError. SFT/trainside callers can reach here mid-training, so guard + # and restore rather than assume the mode. The transformer handle is sufficient: vae and + # vit_model stay in their load-time eval() (bundle.py) — training mode-flips only the + # ``transformer`` trainable module, and siglip has no signature dispatch anyway. + mot = self.bundle.transformer + was_training = mot.training + mot.eval() + try: + with torch.no_grad(), self._autocast_ctx(): + if image is not None: + gen = self._update_context_image(self._resize_input_image(image), gen, vae=True, vit=True) + cfg_text = deepcopy(gen) # snapshot before the prompt text → drop-text branch + gen = inf.update_context_text(prompt, gen) + cfg_img = inf.update_context_text(prompt, cfg_img) + finally: + mot.train(was_training) return gen, cfg_text, cfg_img def _t2i_cache_enabled(self) -> bool: