Skip to content

Conversation

@dhiltgen
Copy link

Proposed changes

This PR extracts the non-CUDA portions of #2972 for Windows support.

Test results (known GGUF library incompatibility on Windows)

(.venv) PS C:\Users\danie\code\mlx> .\build\tests\Release\tests.exe 
[doctest] doctest version is "2.4.12"
[doctest] run with "--help" for options
===============================================================================
C:\Users\danie\code\mlx\tests\load_tests.cpp(43):
TEST CASE:  test gguf

C:\Users\danie\code\mlx\tests\load_tests.cpp(43): ERROR: test case THREW exception: [save_gguf] Compile with MLX_BUILD_GGUF=ON to enable GGUF support.

===============================================================================
C:\Users\danie\code\mlx\tests\load_tests.cpp(97):
TEST CASE:  test gguf metadata

C:\Users\danie\code\mlx\tests\load_tests.cpp(97): ERROR: test case THREW exception: [save_gguf] Compile with MLX_BUILD_GGUF=ON to enable GGUF support.

===============================================================================
[doctest] test cases:  221 |  219 passed | 2 failed | 0 skipped
[doctest] assertions: 3045 | 3045 passed | 0 failed |
[doctest] Status: FAILURE!

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

Copy link
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

The documentation generator does not seem to recognize the macro:

/home/runner/work/mlx/mlx/docs/src/cpp/ops.rst:6: WARNING: Error when parsing function declaration.
If the function has no return type:
  Error in declarator or parameters-and-qualifiers
  Invalid C++ declaration: Expecting "(" in parameters-and-qualifiers. [error at 8]
    MLX_API array roll (const array &a, const Shape &shift, const std::vector< int > &axes, StreamOrDevice s={})

mlx/random.cpp Outdated
T f = T(1.0);
uint16_t* m = (uint16_t*)&f;
*m -= 1;
f.bits_ -= 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work on mac:

  /Users/runner/actions-runner/_work/mlx/mlx/mlx/random.cpp:92:4: error: member reference base type '__fp16' is not a structure or union
     92 |   f.bits_ -= 1;
        |   ~^~~~~~
  /Users/runner/actions-runner/_work/mlx/mlx/mlx/random.cpp:128:22: note: in instantiation of function template specialization 'mlx::core::random::below_one<__fp16>' requested here
    128 |         return array(below_one<float16_t>(), float32);
        |                      ^

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a good pattern for this one, so I backed it out. As a result, there's one known test failure:

C:\Users\danie\code\mlx\tests\random_tests.cpp(244):
TEST CASE:  test random uniform

C:\Users\danie\code\mlx\tests\random_tests.cpp(368): ERROR: CHECK( abs(float(mean(out).item<bfloat16_t>()) - 0.5f) < 0.02 ) is NOT correct!
  values: CHECK( 0.244141 <  0.02 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can reproduce this failure on Mac running CPP tests with CPU, and it is also failing on Windows. /cc @awni

@zcbenz
Copy link
Collaborator

zcbenz commented Jan 20, 2026

Properly exporting public APIs on Windows requires adding a MLX_API macro before every public function, which is a common practice. I'm good with that but also /cc @awni @angeloskath @davidkoski since it would change how we define APIs.

Copy link
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for putting this together!

We still need a few more reviews before merging though.

set(target "${src_name}")
add_executable(${target} ${SRCFILE})
target_link_libraries(${target} PRIVATE mlx)
# On Windows, copy the mlx DLL to the executable directory for runtime loading
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope there is a better solution but I'm good with what it is now. I have created a issue to track this: #3031.

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