Skip to content

Honor explicit BONSAI_VARIANT during M5 metallib swap gating in setup.sh#23

Merged
khosravipasha merged 1 commit into
mainfrom
copilot/fix-issue-6
Jun 14, 2026
Merged

Honor explicit BONSAI_VARIANT during M5 metallib swap gating in setup.sh#23
khosravipasha merged 1 commit into
mainfrom
copilot/fix-issue-6

Conversation

Copilot AI commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

On M5 systems with the NAX miscompile, setup.sh could detect the issue but still skip the ternary metallib swap when both model variants existed locally. The script unconditionally re-inferred binary from models/ and overrode an explicit BONSAI_VARIANT=ternary.

  • Variant resolution behavior

    • Keep _variant="${BONSAI_VARIANT:-ternary}" as the primary source of truth.
    • Only run binary directory auto-detection when BONSAI_VARIANT is unset.
  • Impact on swap path

    • Preserves current auto-detect behavior for default runs.
    • Allows explicit BONSAI_VARIANT=ternary to reach the ternary metallib swap branch instead of being silently forced to binary.
_variant="${BONSAI_VARIANT:-ternary}"
if [ -z "${BONSAI_VARIANT:-}" ]; then
  for _bin_model in "$SCRIPT_DIR"/models/bonsai-image-4B-binary-*; do
    [ -d "$_bin_model" ] && _variant="binary" && break
  done
fi

Copilot AI changed the title Fix setup.sh to honor explicit BONSAI_VARIANT during M5 metallib swap check Honor explicit BONSAI_VARIANT during M5 metallib swap gating in setup.sh Jun 14, 2026
Copilot AI requested a review from khosravipasha June 14, 2026 05:37
@khosravipasha khosravipasha requested a review from Copilot June 14, 2026 05:41
@khosravipasha khosravipasha marked this pull request as ready for review June 14, 2026 05:41
@khosravipasha khosravipasha requested a review from bri-prism June 14, 2026 05:42

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.

Pull request overview

This PR fixes setup.sh variant resolution so an explicitly set BONSAI_VARIANT is not overridden by local models/ auto-detection, ensuring the intended metallib swap gating behavior on affected M5 systems.

Changes:

  • Preserve BONSAI_VARIANT as the primary source of truth for _variant.
  • Run binary-model auto-detection only when BONSAI_VARIANT is unset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread setup.sh
Comment on lines 354 to +358
# to swap whenever a 1-bit model is already present under models/. Binary users
# who run setup before downloading a model should export BONSAI_VARIANT=binary.
_variant="${BONSAI_VARIANT:-ternary}"
for _bin_model in "$SCRIPT_DIR"/models/bonsai-image-4B-binary-*; do
[ -d "$_bin_model" ] && _variant="binary" && break
done
if [ -z "${BONSAI_VARIANT:-}" ]; then
for _bin_model in "$SCRIPT_DIR"/models/bonsai-image-4B-binary-*; do

@bri-prism bri-prism 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.

The fix is correct and nicely scoped. Gating the binary auto-detection behind [ -z "${BONSAI_VARIANT:-}" ] is the right call, since an explicit BONSAI_VARIANT=ternary now reaches the metallib swap branch instead of being clobbered when a binary model happens to be sitting in models/.

I walked through the three cases and they all hold:

  • Unset: defaults to ternary, then auto-detect can promote it to binary, same as before.
  • ternary: stays ternary, detection skipped. That is the case we wanted to fix.
  • binary: stays binary, detection skipped.

One minor note, not a blocker: when no binary model is present the glob still falls through cleanly because the directory test rejects the literal pattern, so that path is unchanged.

Looks good to me.

@khosravipasha khosravipasha merged commit 9cf9d6e into main Jun 14, 2026
4 checks passed
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.

4 participants