Skip to content

Build win arm64#1944

Merged
aaronvg merged 12 commits intocanaryfrom
baml-py-win
May 14, 2025
Merged

Build win arm64#1944
aaronvg merged 12 commits intocanaryfrom
baml-py-win

Conversation

@aaronvg
Copy link
Copy Markdown
Contributor

@aaronvg aaronvg commented May 13, 2025

Important

Add support for building Python wheels for Windows ARM64 in GitHub Actions workflows and update pyproject.toml for compatibility.

  • Build System:
    • Add ARM64 Windows target to build-python-release.reusable.yaml.
    • Setup build environment for ARM64 Windows, including Rust and Python 3.11, 3.12, 3.13.
    • Update pyproject.toml to include pyo3/generate-import-lib feature for Windows ARM64.
  • Workflows:
    • Modify release.yml to expect at least 8 wheels, reflecting the new ARM64 build.
    • Minor formatting changes in release.yml.

This description was created by Ellipsis for 63d12a4. You can customize this summary. It will automatically update as commits are pushed.

@vercel
Copy link
Copy Markdown

vercel bot commented May 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 5:22am

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 85c493e in 1 minute and 15 seconds. Click for details.
  • Reviewed 24 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:5
  • Draft comment:
    Mixing 'workflow_call' with a push trigger is unusual. Confirm that triggering on branch 'baml-py-win' is intended.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. .github/workflows/build-python-release.reusable.yaml:55
  • Draft comment:
    For the new aarch64 Windows target, the setup-python step always sets architecture to 'x64'. Adjust if an arm64 python is needed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_6zkXPbV5JXVLDLmI

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 1803663 in 1 minute and 44 seconds. Click for details.
  • Reviewed 103 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:79
  • Draft comment:
    Remove extra spaces in the 'needs' array for consistency.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. .github/workflows/release.yml:130
  • Draft comment:
    Wheel count threshold updated from 7 to 8; please confirm this meets all build expectations.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm if the change meets build expectations, which is against the rules. It doesn't provide a specific suggestion or point out a potential issue with the code itself.
3. .github/workflows/release.yml:308
  • Draft comment:
    Removed trailing whitespace from artifact pattern strings; ensure these patterns still match the intended artifacts.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. .github/workflows/release.yml:369
  • Draft comment:
    Added a newline at the end of file for POSIX compliance.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
5. engine/language_client_python/pyproject.toml:19
  • Draft comment:
    Added 'pyo3/generate-import-lib' feature; verify it works correctly on all target platforms, especially non-Windows.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to verify the functionality of a feature across different platforms. It doesn't provide a specific suggestion or point out a specific issue in the code. It falls under the rule of not asking the author to ensure the behavior is intended or tested.
6. engine/language_client_python/pyproject.toml:18
  • Draft comment:
    Typographical suggestion: Consider capitalizing 'Windows' in the comment on line 18 (i.e., change 'fix windows aarch64' to 'fix Windows aarch64') for improved clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that "Windows" should be capitalized, this is an extremely minor stylistic issue in a comment. It doesn't affect functionality, readability, or maintainability in any meaningful way. The comment's meaning is perfectly clear either way. Perhaps maintaining consistent capitalization across the codebase is important for professionalism and attention to detail? While consistency is good, this level of nitpicking on comments is more likely to be distracting than helpful, and goes against our rule about not making purely informative or unimportant comments. Delete this comment as it's too minor and doesn't affect code quality or functionality in any meaningful way.

Workflow ID: wflow_hpgb5OV0KIs8ByrO

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed c38d170 in 1 minute and 36 seconds. Click for details.
  • Reviewed 30 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:57
  • Draft comment:
    The 'interpreter' field for aarch64-pc-windows-msvc is set to a space‐separated string ('3.11 3.12 3.13'). This may not be parsed as multiple interpreters. If multiple interpreter builds are intended, consider using separate matrix entries or a proper list.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The key is to understand how maturin-action handles the --interpreter argument. Looking at line 76, the format string suggests this is passed directly to a command line tool. Without knowing maturin's documentation or implementation, I can't be certain whether space-separated values are valid or not. The fact that this syntax was chosen deliberately suggests it might work, but I can't be 100% sure either way. I don't have access to maturin's documentation to verify if this syntax is valid or not. The PR author may have tested this and found it works. Given the uncertainty and lack of strong evidence that this is actually incorrect, we should err on the side of assuming the author tested this syntax. Delete the comment since we don't have strong evidence that the current syntax is problematic, and the author likely tested this configuration.
2. .github/workflows/build-python-release.reusable.yaml:65
  • Draft comment:
    The conditional for 'python-version' always selects '3.11' for aarch64-pc-windows-msvc, which contradicts the multiple versions provided in the interpreter field. Confirm if this is intentional.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The setup-python action (3.11) and the maturin build interpreters (3.11, 3.12, 3.13) serve different purposes. The setup-python is just for running the build environment, while maturin can build wheels for multiple Python versions from a single Python environment. This is a common pattern and not a bug. Maybe there's a reason they need the build environment Python version to match one of the target versions? Maybe using 3.8 would work better? No, the build environment Python version is independent of the target wheel versions. Using Python 3.11 for aarch64 Windows builds is likely intentional due to compatibility requirements. This comment should be deleted as it points out a non-issue. The different Python versions for setup vs build are intentional and correct.
3. .github/workflows/build-python-release.reusable.yaml:76
  • Draft comment:
    Appending '--interpreter' with a space-separated list (if provided) may not work as expected with maturin. Verify whether maturin supports multiple interpreters in a single invocation or consider iterating over each interpreter.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid concern about command-line argument handling. However, it's speculative ("may not work") and asks for verification rather than making a clear assertion. The comment is phrased as a request to verify behavior rather than pointing out a definite issue. Looking at maturin docs would help, but that would require external context we don't have. The comment might be identifying a real technical issue with maturin's argument handling. If maturin doesn't support multiple interpreters, this could cause build failures. While the concern might be valid, the comment violates our rules by asking for verification ("Verify whether...") instead of making a clear assertion about a problem. It's also speculative about potential issues. The comment should be deleted because it's phrased as a verification request and makes speculative claims without clear evidence of an actual problem.

Workflow ID: wflow_8td0eCTwajI93ocD

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed a682f11 in 47 seconds. Click for details.
  • Reviewed 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:90
  • Draft comment:
    Artifact name now includes interpreter info. Verify that matrix._.interpreter is always a string and not an array; if it might be an array, consider explicitly handling or quoting it.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_gaJhGyOmUSa1GFSM

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed e6f3094 in 1 minute and 36 seconds. Click for details.
  • Reviewed 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:70
  • Draft comment:
    Hardcoding XWIN_VERSION may prevent passing additional matrix env vars or be applied on non-Windows targets. Consider conditionally setting it for Windows builds or merging with matrix.env.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment raises two concerns: 1) blocking matrix.env vars and 2) applying to non-Windows builds. Looking at the diff history, matrix.env was removed in favor of this explicit setting. The maturin issue link suggests this is an intentional fix. The env var being set unconditionally for all targets is likely not an issue since non-Windows builds would simply ignore it. I could be wrong about non-Windows builds safely ignoring the env var. There might be edge cases where this causes problems. The linked maturin issue and the fact this is a common build tool suggests this is a safe approach. If it caused issues on non-Windows platforms, it would likely be widely known. The comment should be deleted as it raises theoretical concerns without strong evidence of actual problems, and the change appears to be an intentional fix referenced by the maturin issue.

Workflow ID: wflow_hOdvlaHm4p6BxT0z

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 649e5cc in 48 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:56
  • Draft comment:
    Ensure 'windows-11-arm' is a valid/available runner and that any runner-specific settings (e.g. Python architecture) are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that a specific runner is valid and that settings are updated accordingly. This falls under the category of asking the author to double-check things, which is against the rules.

Workflow ID: wflow_wbxAbf6bwWT63WyR

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed bd3469a in 1 minute and 57 seconds. Click for details.
  • Reviewed 67 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:75
  • Draft comment:
    Avoid using root '/' for downloads; use a temporary or dedicated directory.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a GitHub Actions workflow running in an isolated container environment. While using root paths isn't ideal practice, in this context it's temporary, isolated, and commonly done in CI scripts. The files are only used for setup and don't persist. The security and organization concerns that normally apply to root directory usage are less relevant here. The comment raises a valid general best practice. Using root paths could potentially cause permission issues or conflicts with other processes. In a CI environment, these concerns are mitigated since it's an isolated, temporary container that's discarded after the job completes. The current approach is simple and functional. While the comment suggests a valid best practice, the current approach is acceptable for a CI environment and changing it would add unnecessary complexity without significant benefit.
2. .github/workflows/build-python-release.reusable.yaml:83
  • Draft comment:
    Inconsistent file path: the vs_BuildTools.exe is saved as '/vs_BuildTools.exe' but later referenced as 'C:/vs_BuildTools.exe'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% In Windows PowerShell, "/" at the start of a path is equivalent to "C:/". This is standard behavior and the code will work fine as-is. The comment is technically pointing out a style inconsistency, but it's not a functional issue and both forms are valid. The code is part of a newly added ARM64 Windows setup block. The comment does identify an actual inconsistency in path style that could be made more uniform for better readability and maintenance. While there is a style inconsistency, it doesn't affect functionality and isn't significant enough to warrant a code change. Both path formats are valid and commonly used in PowerShell. The comment should be deleted as it points out a non-issue - both path formats are valid in PowerShell and the code works correctly as written.
3. .github/workflows/build-python-release.reusable.yaml:97
  • Draft comment:
    jq.exe is downloaded using an i386 build, which may not be appropriate for an ARM64 target.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% 32-bit x86 programs can generally run on ARM64 Windows through emulation. jq is a simple command-line tool used for JSON processing, not a performance-critical component. The suggested ARM64 URL might not even exist - I don't see clear evidence that jq provides ARM64 Windows builds. The tool seems to be used only for tomlq/yq functionality, which is likely just configuration processing. While architecturally inconsistent, the i386 version will work through Windows' emulation layer. Without confirming that an ARM64 version exists and works, suggesting the change could be counterproductive. The comment suggests a URL that may not exist and proposes fixing an issue that likely isn't causing any real problems in practice. The comment should be removed. The i386 version of jq will work fine through Windows' x86 emulation, and there's no clear evidence that an ARM64 version is available or necessary.
4. .github/workflows/build-python-release.reusable.yaml:104
  • Draft comment:
    Appending 'C:/' to GITHUB_PATH seems overly broad; consider only adding specific bin directories to avoid path conflicts.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Adding "C:/" to PATH is indeed very broad and could potentially cause conflicts by making all executables in C:/ root directly accessible. However, this is in a controlled CI environment where we know exactly what's installed. The "C:/" path is added specifically to find jq.exe which was installed there. The script shows clear intention about what tools are being installed and where. The comment raises a valid security/stability concern about PATH manipulation. Adding root directories to PATH is generally considered bad practice. While the concern is valid in general, this is a controlled CI environment with known installed tools. The broad PATH addition is intentional to locate jq.exe which was specifically installed in C:/. The comment should be deleted because the broad PATH addition is intentional for accessing jq.exe in this controlled CI environment. The potential risks are acceptable in this context.
5. .github/workflows/build-python-release.reusable.yaml:84
  • Draft comment:
    The multiline argument list for Start-Process is fragile; consider using a YAML multiline string or variable for better readability.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
6. .github/workflows/build-python-release.reusable.yaml:112
  • Draft comment:
    Ensure that UNIX tools (bash, clang) are available on ARM64 Windows; the check step may fail if these are absent or misconfigured.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that certain tools are available, which falls under the rule of not asking the author to ensure or verify things. It does not provide a specific code suggestion or point out a specific issue in the code.

Workflow ID: wflow_OSwD0gUw6PSotKCl

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 070a430 in 1 minute and 19 seconds. Click for details.
  • Reviewed 35 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:60
  • Draft comment:
    Clarify the reference comment: add brief context on why the polars workflow is relevant.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking for clarification on a reference comment, which seems to be asking for more information or context. This falls under the rule of not asking the PR author to update the PR description or provide additional context, which is not allowed.

Workflow ID: wflow_g4GJoxpmSYlet17W

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.


Get-Content $Env:GITHUB_PATH | Out-Host

# - name: Check build environment (ARM64 Windows)
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.

If the build environment check step is no longer needed, remove it instead of leaving commented-out code or document why it's disabled.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 3ad2bba in 1 minute and 44 seconds. Click for details.
  • Reviewed 100 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:14
  • Draft comment:
    Global default shell set to bash is fine, but ensure any step needing PowerShell (like 'Setup build environment') explicitly overrides it.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. .github/workflows/build-python-release.reusable.yaml:62
  • Draft comment:
    Ensure 'architecture: arm64' is correctly supported by actions (e.g. setup-python) and handled for non-ARM entries.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. .github/workflows/build-python-release.reusable.yaml:75
  • Draft comment:
    Removal of setup steps for Git Bash, VS Build Tools, CMake, and jq may break later commands (e.g. 'bash --version', 'clang -v', 'cmake --version'). Verify that these dependencies are available on the windows-11-arm runner.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify the availability of certain dependencies on a specific runner. This falls under the category of asking the author to double-check or ensure something, which is against the rules. Therefore, this comment should be removed.
4. .github/workflows/build-python-release.reusable.yaml:97
  • Draft comment:
    Using matrix._.architecture for 'setup-python' may be undefined for other targets. Consider conditionally setting this input only for Windows ARM builds.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid point - matrix._.architecture is only defined for one target but is used unconditionally. However, looking at actions/setup-python docs, the architecture parameter is optional and will default appropriately if not specified. So while technically correct about being undefined, it's not actually a problem in practice since undefined is handled gracefully. I could be wrong about the setup-python action's behavior with undefined architecture. There could be edge cases where an undefined architecture causes issues. The setup-python action is widely used and well-tested. Its documentation indicates architecture is optional, so undefined values should be handled properly. While technically correct, this comment points out something that isn't actually a problem due to how setup-python handles undefined architecture values. The comment should be removed as it suggests fixing something that isn't broken.

Workflow ID: wflow_Qvn4kIaqqRMeL78r

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed e192a27 in 1 minute and 47 seconds. Click for details.
  • Reviewed 96 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:24
  • Draft comment:
    Avoid leaving large blocks of commented-out matrix targets. If these builds aren’t needed, consider removing them to reduce clutter.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid code quality point - keeping large blocks of commented code can make files harder to maintain and read. However, in this case, the comments appear to be intentionally preserved as reference configuration for other platforms, complete with explanatory notes and links to issues. This suggests they serve as valuable documentation for future re-enablement of these targets. The commented code might actually be serving as important documentation for complex build configurations. The author may have intentionally left these as templates for future use. While commented code can be problematic, in CI configuration files it's common to temporarily disable certain build targets while preserving their configuration for future reference, especially when the configuration is complex and well-documented. The comment should be deleted as the commented code appears to serve a legitimate documentation purpose in this CI configuration context.
2. .github/workflows/build-python-release.reusable.yaml:59
  • Draft comment:
    The matrix entry includes an 'interpreter' field (e.g. '3.11 3.12 3.13') that is no longer used in the build step. Remove or update it to avoid confusion.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_L5uapSFRVxMs1xkv

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

# building in engine/ ensures that we pick up .cargo/config.toml
working-directory: engine
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml --interpreter ${{ steps.python311.outputs.python-path }}
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.

Output keys containing hyphens should use bracket notation. Consider using steps.python311.outputs['python-path'] instead of dot notation.

Suggested change
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml --interpreter ${{ steps.python311.outputs.python-path }}
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml --interpreter ${{ steps.python311.outputs['python-path'] }}

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 5577564 in 1 minute and 28 seconds. Click for details.
  • Reviewed 97 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:95
  • Draft comment:
    Renamed step id from 'python311' to 'python-setup'. Ensure that all references (e.g. in the Build wheels step) are updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_qcj73V195GuUpRkv

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

# building in engine/ ensures that we pick up .cargo/config.toml
working-directory: engine
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml ${{ matrix._.target == 'aarch64-pc-windows-msvc' && format('--interpreter {0}', steps.python-setup.outputs.python-path) || '' }}
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.

GitHub Actions expressions do not support a format function. Consider using concat (e.g. concat('--interpreter ', steps.python-setup.outputs.python-path)) to construct the argument string.

Suggested change
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml ${{ matrix._.target == 'aarch64-pc-windows-msvc' && format('--interpreter {0}', steps.python-setup.outputs.python-path) || '' }}
args: --release --out language_client_python/dist --manifest-path language_client_python/Cargo.toml ${{ matrix._.target == 'aarch64-pc-windows-msvc' && concat('--interpreter ', steps.python-setup.outputs.python-path) || '' }}

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 63d12a4 in 1 minute and 33 seconds. Click for details.
  • Reviewed 73 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-python-release.reusable.yaml:60
  • Draft comment:
    Removed the 'interpreter' field for ARM64 target. Confirm that the new multi-version setup (separate Python steps) fully replaces its use.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. .github/workflows/build-python-release.reusable.yaml:97
  • Draft comment:
    The default Python setup uses version 3.8 with its architecture taken from the matrix. Ensure that for non-ARM targets the 'architecture' value is defined or remove it if not needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. .github/workflows/build-python-release.reusable.yaml:135
  • Draft comment:
    When formatting interpreter paths in the build wheels args, consider quoting each path to handle possible spaces in directory names.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. .github/workflows/build-python-release.reusable.yaml:152
  • Draft comment:
    Artifact name now omits interpreter details. If multiple Python versions are built (as on ARM64 Windows), consider including interpreter info for clearer artifact identification.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_1labTUvElLGlw5pb

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@aaronvg aaronvg added this pull request to the merge queue May 14, 2025
Merged via the queue into canary with commit ef9e8be May 14, 2025
21 checks passed
@aaronvg aaronvg deleted the baml-py-win branch May 14, 2025 05:44
@ellipsis-dev ellipsis-dev bot mentioned this pull request May 14, 2025
ba11b0y pushed a commit that referenced this pull request May 16, 2025
<!-- ELLIPSIS_HIDDEN -->


> [!IMPORTANT]
> Add support for building Python wheels for Windows ARM64 in GitHub
Actions workflows and update `pyproject.toml` for compatibility.
> 
>   - **Build System**:
> - Add ARM64 Windows target to `build-python-release.reusable.yaml`.
> - Setup build environment for ARM64 Windows, including Rust and Python
3.11, 3.12, 3.13.
> - Update `pyproject.toml` to include `pyo3/generate-import-lib`
feature for Windows ARM64.
>   - **Workflows**:
> - Modify `release.yml` to expect at least 8 wheels, reflecting the new
ARM64 build.
>     - Minor formatting changes in `release.yml`.
> 
> <sup>This description was created by </sup>[<img alt="Ellipsis"
src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup>
for 63d12a4. You can
[customize](https://app.ellipsis.dev/BoundaryML/settings/summaries) this
summary. It will automatically update as commits are pushed.</sup>


<!-- ELLIPSIS_HIDDEN -->
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.

1 participant