Conversation
|
Important Review skippedToo many files! This PR contains 211 files, which is 111 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (211)
You can disable this status message by setting the 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 |
Remove pstack-style intros, broken handoff links, and stale README references. No functional code changes. Co-authored-by: Aryan Kumar <aryan5v@users.noreply.github.com>
cursor
Bot
changed the title
Strip narrative comments, section banners, and YAML header prose across the PR scope. Shorten analysis script module docstrings. No logic or CI changes. Co-authored-by: Aryan Kumar <aryan5v@users.noreply.github.com>
Collapse quantization docs to operational minimums, shorten quant module docstrings, and trim CompactH3 index. No logic or CI changes. Co-authored-by: Aryan Kumar <aryan5v@users.noreply.github.com>
Adds the centered-affine rank compression and behavioral sweep that were missing from the consolidation: fold emission, the runtime AdaLN patch, the hard-motion metric set, rank comparison, and the identity gate. Adds hard-motion eval set (14 cases) and the r768/r16 QAD configs. Cluster roots are parameterized (COMPACTH3_ROOT, COMPACTH3_EVAL_ROOT, COMPACTH3_CODE_ROOT) so the scripts run outside the original tree.
aryan5v
marked this pull request as ready for review
September 19, 2026 13:05
Owner
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import numpy as np | ||
| import cv2 | ||
|
|
||
| EVAL_DIR = Path(os.environ.get("COMPACTH3_EVAL_ROOT", str(Path(SPRINT_ROOT).parent / "fasth3-eval"))) |
There was a problem hiding this comment.
This module evaluates os.environ without importing os and also references the undefined Python global SPRINT_ROOT. sweep_driver.py and emit_folds.py contain the same undefined global. Because the launchers execute these files directly without injecting that name, the sweep and measurement commands raise NameError before argument parsing and never start.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/compacth3/sweep/measure_rank.py
Line: 13
Comment:
**Sweep scripts fail at startup**
This module evaluates `os.environ` without importing `os` and also references the undefined Python global `SPRINT_ROOT`. `sweep_driver.py` and `emit_folds.py` contain the same undefined global. Because the launchers execute these files directly without injecting that name, the sweep and measurement commands raise `NameError` before argument parsing and never start.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| SP_SIZE="${SP_SIZE:-1}" | ||
| HSDP_REPLICATE="${HSDP_REPLICATE:-1}" | ||
| HSDP_SHARD="${HSDP_SHARD:-${WORLD_SIZE}}" | ||
| CONFIG="${CONFIG:-examples/train/configs/distribution_matching/minimax_h3/dmd2_sp1_fsdp40_vidprom_v6.yaml}" |
There was a problem hiding this comment.
The default points to dmd2_sp1_fsdp40_vidprom_v6.yaml, but that file is not present in the added MiniMax-H3 configuration directory. Running this launcher without manually setting CONFIG therefore passes a nonexistent file to examples/train/run.sh and aborts before training begins.
Prompt To Fix With AI
This is a comment left during a code review.
Path: examples/distill/MiniMax-H3/distill_dmd.sh
Line: 17
Comment:
**Default config is missing**
The default points to `dmd2_sp1_fsdp40_vidprom_v6.yaml`, but that file is not present in the added MiniMax-H3 configuration directory. Running this launcher without manually setting `CONFIG` therefore passes a nonexistent file to `examples/train/run.sh` and aborts before training begins.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| cfg['method']['feature_local_block_indices'] = [i for i in range(1, len(original)) if original[i] > original[i-1]+1] | ||
| config_path = a.output.resolve() / 'stage34.yaml' | ||
| config_path.write_text(yaml.safe_dump(cfg, sort_keys=False)) | ||
| script = (root / 'scripts/fasth3_sprint/slurm_h3_base42_recovery.sbatch').read_text() |
There was a problem hiding this comment.
This unconditionally reads scripts/fasth3_sprint/slurm_h3_base42_recovery.sbatch, which is absent from this revision; only the differently named slurm_h3_base42_recovery500.sbatch is checked in. Every stage-34 preparation therefore raises FileNotFoundError after already creating a partial output directory.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/fasth3_sprint/prepare_h3_stage34.py
Line: 33
Comment:
**Stage launcher path is missing**
This unconditionally reads `scripts/fasth3_sprint/slurm_h3_base42_recovery.sbatch`, which is absent from this revision; only the differently named `slurm_h3_base42_recovery500.sbatch` is checked in. Every stage-34 preparation therefore raises `FileNotFoundError` after already creating a partial output directory.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+501
to
+508
| else: | ||
| last_log = time.monotonic() | ||
| while not export_status_path.is_file(): | ||
| time.sleep(2.0) | ||
| now = time.monotonic() | ||
| if now - last_log >= 60.0: | ||
| logger.info("Waiting for rank-zero inference export at step %s", step) | ||
| last_log = now |
There was a problem hiding this comment.
If rank-zero staging, dcp.save, or publishing export-status.json fails before status propagation, surviving ranks either wait at a barrier or poll for this file forever. This loop has no timeout, so a one-rank filesystem or DCP failure can indefinitely block the entire distributed training job.
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/train/utils/checkpoint.py
Line: 501-508
Comment:
**Checkpoint failures can deadlock**
If rank-zero staging, `dcp.save`, or publishing `export-status.json` fails before status propagation, surviving ranks either wait at a barrier or poll for this file forever. This loop has no timeout, so a one-rank filesystem or DCP failure can indefinitely block the entire distributed training job.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| for dst, src in zip(acc, rows, strict=True): | ||
| for metric in METRICS: | ||
| dst[metric] += float(src[metric]) | ||
| return {cat: [{m: v / count for m, v in row.items()} for row in rows] for cat, rows in totals.items()} |
There was a problem hiding this comment.
The producer stores per-category sums and emits zero accumulators when a category is absent from a shard, but this code divides those sums by the total number of shard files. Sparse categories such as the eight-example multiple_shots group are therefore diluted relative to other categories. The resulting importance scores can rank blocks incorrectly and affect the model's permanent pruning map; the canonical aggregator instead divides by each category's actual sample count.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/fasth3_sprint/select_h3_block_map.py
Line: 32
Comment:
**Shard count biases pruning**
The producer stores per-category sums and emits zero accumulators when a category is absent from a shard, but this code divides those sums by the total number of shard files. Sparse categories such as the eight-example `multiple_shots` group are therefore diluted relative to other categories. The resulting importance scores can rank blocks incorrectly and affect the model's permanent pruning map; the canonical aggregator instead divides by each category's actual sample count.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
763
to
+764
| if interval is None: | ||
| interval = 1 | ||
| interval = 5 |
There was a problem hiding this comment.
An omitted generator_update_interval now resolves to five, while the public training documentation and example configuration still state that the default is one. Users relying on that documented default will unknowingly run four critic-only iterations between student updates, so the documentation must be updated with this behavior change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/train/methods/distribution_matching/dmd2.py
Line: 763-764
Comment:
**Documented cadence default is stale**
An omitted `generator_update_interval` now resolves to five, while the public training documentation and example configuration still state that the default is one. Users relying on that documented default will unknowingly run four critic-only iterations between student updates, so the documentation must be updated with this behavior change.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| ) | ||
| from fastvideo.models.utils import set_weight_attrs | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
This new core-package module calls logging.getLogger(__name__) directly. The repository directive requires FastVideo package code to use from fastvideo.logger import init_logger and logger = init_logger(__name__) so formatting and distributed logging behavior remain consistent. This repository requirement must be satisfied before merging.
Context Used: fastvideo/AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/layers/quantization/int8_affine_config.py
Line: 22
Comment:
**Logger bypasses shared initializer**
This new core-package module calls `logging.getLogger(__name__)` directly. The repository directive requires FastVideo package code to use `from fastvideo.logger import init_logger` and `logger = init_logger(__name__)` so formatting and distributed logging behavior remain consistent. This repository requirement must be satisfied before merging.
**Context Used:** fastvideo/AGENTS.md ([source](https://github.com/aryan5v/fastvideo/blob/main/fastvideo/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch has not been deployed
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
2 participants
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
Lineage represented
The promoted 42-block path uses activation-guided block selection, recovery through the selected checkpoint-750 parent, and the corrected DMD2 run whose selected checkpoint is 1400. The 34-block model is derived from the recovered 42-block model and currently includes recovery infrastructure only; it has not yet been promoted through DMD2.
Validation performed
git diff --cached --checkbash -nfor every changed shell and Slurm launcherRemaining validation
This is intentionally a draft. Targeted CPU/GPU tests, cluster path parameterization, one clean recovery/DMD2 launch smoke, and final removal of any redundant historical sprint scripts remain before promotion.
This PR is not safe to merge until the broken launch paths, distributed inference-checkpoint deadlock, and pruning-score normalization are corrected.
Fix with agent prompt
Summary
This PR introduces the CompactH3 recovery and pruning workflow, joint audio-video DMD2 training and export infrastructure, native-shape preprocessing, validation media logging, and several low-bit quantization formats. The review found startup failures in newly added launch tooling, a distributed checkpoint deadlock path, and biased pruning-score aggregation.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Native T2VA / text data] --> B[Exact-shape dataloader] B --> C[CompactH3 recovery] C --> D[42-block recovered parent] D --> E[Four-call DMD2] E --> F[Training DCP checkpoints] F --> G[Inference checkpoint export] G --> H[Validation and checkpoint grading] D --> I[34-block recovery preparation] E --> J[INT8 / NVFP4 / W4A16 export]Reviews (1) · Last reviewed commit: "[feat]: add AdaLN rank sweep and rank-sp..."