Skip to content

fix(mlx): address cache recovery and Python compatibility reviews - #927

Merged
DevSecNinja merged 2 commits into
mainfrom
devsecninja-mlx-review-follow-up
Oct 17, 2026
Merged

DevSecNinja merged 2 commits into
mainfrom
devsecninja-mlx-review-follow-up

Conversation

@DevSecNinja

Copy link
Copy Markdown
Owner

Description

Focused follow-up to #923, including the Copilot review submitted 2026-10-17 20:48:44 UTC, after #923 was merged at 20:42:51 UTC. This PR is intentionally left open and unmerged, with no auto-merge. A passing CI or CodeRabbit check is not evidence that all reviews are complete or findings resolved. Original review threads remain unresolved while these fixes are only in this follow-up.

Changes

  • Check this specific Qwen3.5 snapshot's config, tokenizer, chat template, weight index and every indexed shard before loading. Missing files trigger one resumable download to the same external cache, pinned to the returned cached revision. Check again before loading, preserve existing files, and propagate malformed metadata, download and model-load errors. Restrict snapshot lookup/download to the model files so missing README/other unrelated metadata does not force a download.
  • Require Python 3.11+ consistently in Task preflight, launcher and documentation; a PATH Python 3.10 now falls back to Homebrew before creating a venv. Working installs still skip provisioning/upgrades.
  • Replace all negative ! grep assertions in the focused Bats file with run grep plus an exact exit-status assertion.
  • Replace the GNU-only documented timeout invocation with a small standard-library macOS/Linux runner. It kills the test process group on timeout/Ctrl+C and preserves exit status (124 on timeout, 130 on interrupt, 128 + signal for signal termination).
  • Preserve external-only storage, no-thinking behavior, the 8,192-token admission bound, in-memory conversation state and selective Chezmoi deployment. The repository-root Taskfile is unchanged.

Review dispositions

Finding Disposition in this PR
CodeRabbit: partial snapshots Valid; fixed with model-specific required-file/shard checks and bounded recovery. The dependency example was inaccurate: Transformers 5.7.0 requires Hub >=1.5.0, not >=1.0.0. Supported Hub 1.5.0 still exhibits the reported behavior.
CodeRabbit: ineffective negative assertions Valid; fixed all five occurrences, including the previously effective final-position assertions for consistency.
CodeRabbit: macOS timeout command Valid; a direct subprocess.run(timeout=...) one-liner would not reliably clean up grandchildren, so the runner uses a dedicated process group.
Copilot: Python minimum Valid; aligned to mlx-lm 0.32.2's actual Python >=3.11 requirement and added 3.10 rejection/fallback plus 3.11 acceptance regressions.
Copilot: partial snapshots Valid and overlaps CodeRabbit; recovery stays on the returned cached revision, with subsequent offline reuse.
CodeRabbit top-level 80% docstring-coverage warning Not changed: the review used CodeRabbit defaults; no repository docstring threshold or corresponding lint gate was found. Avoided unrelated comment churn.

Dependency evidence: mlx-lm 0.32.2 metadata, Transformers 5.7.0 metadata, Hub 1.5.0 snapshot implementation. No real model downloads or inference were used.

Validation

  • 22 focused Bats checks pass, including 34 Python tests, fake MLX/Hub dependencies, real Task invocation and selective Chezmoi apply in a temporary home.
  • Reproduced two consecutive missing-shard failures against the original launcher; the fixed launcher resumes once at the cached revision and then uses the cache offline.
  • Injected unexpected --upgrade, create-venv and install-python actions into temporary copies of the actual Bats fixtures: all three mutations wrongly pass before the assertion changes and correctly fail afterward.
  • Core Chezmoi/shell/Fish/dry-run validation: 23 pass, 1 skip. All-files and staged lefthook hooks pass; Conventional Commit verification passes. Every validation run has a hard timeout.
  • The full local suite is not green: 896 tests completed with 32 failures outside the changed MLX checks. These include GNU-tool/path assumptions on macOS, credentials/network/environment assumptions, and existing mock isolation issues. Re-ran a bounded representative subset from an untouched main archive and reproduced 25 of those failures. Left unrelated tests/code unchanged; this is not a claim that the full suite passed.
  • No live-home Chezmoi apply, primary-checkout edits, inference, model downloads or external-cache modifications. Existing tools were already installed. Hook-induced executable-mode changes to unrelated files were removed.

Checklist

  • Linting passes locally (mise exec -- lefthook run pre-commit)
  • Commit messages follow Conventional Commits
  • Documentation updated (if applicable)

Validate Qwen snapshots and resume missing files at the cached revision. Require Python 3.11, repair negative Bats assertions, and document a process-group timeout runner for macOS and Linux.

Co-authored-by: Copilot App <[email protected]>
Copilot-Session: 99d7cebd-e0b2-454f-babb-30613dcb1016
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 18 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a7bd145f-92fe-4e43-b7db-79bacbffba43
📥 Commits

Reviewing files that changed from the base of the PR and between 6b07e2e and d06fd66.

📒 Files selected for processing: 7
  • docs/development-tools.md
  • home/Taskfile.yml
  • home/dot_config/mlx/chat.py
  • tests/bash/global-mlx-chat.bats
  • tests/python/test_mlx_chat.py
  • tests/python/test_timeout.py
  • tests/with-timeout.py
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Closed output pipes can bypass timeout cleanup, and missing Python causes setup failures instead of test skips.

2 open issues
What changed in this PR

Follows up on #923 to improve global MLX chat cache recovery, Python compatibility, and portable validation.

Changes:

  • Validate cached model files and retry incomplete snapshots at the cached revision.
  • Require Python 3.11+ consistently.
  • Strengthen regression checks and add a portable process-group timeout runner.
File Description
tests/​with-timeout.py Adds portable timeout and interrupt cleanup.
tests/​python/​test_timeout.py Tests timeout cleanup and exit statuses.
tests/​python/​test_mlx_chat.py Covers cache recovery and Python compatibility.
tests/​bash/​global-mlx-chat.bats Strengthens assertions and Python fallback checks.
home/​Taskfile.yml Raises Python preflight minimum to 3.11.
home/​dot_config/​mlx/​chat.py Validates snapshots and retries missing downloads.
docs/​development-tools.md Documents recovery, requirements, and portable validation.

Review effort: Balanced

Comment thread tests/with-timeout.py Outdated
Comment thread tests/bash/global-mlx-chat.bats Outdated
Terminate and reap validation children before best-effort unbuffered stderr diagnostics. Cover closed stdout and stderr during timeout and interruption, and skip the MLX fixture when Python is unavailable.

Co-authored-by: Copilot App <[email protected]>
Copilot-Session: 99d7cebd-e0b2-454f-babb-30613dcb1016
@DevSecNinja
DevSecNinja merged commit 0a6f919 into main Oct 17, 2026
20 checks passed
@DevSecNinja
DevSecNinja deleted the devsecninja-mlx-review-follow-up branch October 17, 2026 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants