ml-explore / mlx

MLX: An array framework for Apple silicon
https://ml-explore.github.io/mlx/
MIT License
15.01k stars 856 forks source link

Resolves build issues with the extension example #419

Closed dc-dc-dc closed 4 months ago

dc-dc-dc commented 4 months ago

Proposed changes

Updates extension example with new api, and fixes the build issues with json headers missing.

Also added the build example to ci, ideally should run ci test in this pr before committing to make sure its good.

Checklist

Put an x in the boxes that apply.

angeloskath commented 4 months ago

This looks good to me. I am not sure we want to build the example extension at every commit, haven't decided yet. @jagrit06 take a look when you time and approve.

dc-dc-dc commented 4 months ago

I think having the examples be part of the test suite makes sense as it ensures that they don't get stale, and developers looking to build on them don't end up running into issues due to missing updates.

awni commented 4 months ago

I'm ok with adding it to CircleCI for now. Nevertheless, this should still be viewed as an experimental feature, we may break compatibility with it in the future (in favor of alternative extension approaches). It's one reason we don't advertise it much in the docs.

dc-dc-dc commented 4 months ago

rebased, but before merging can we do a quick ci run in this pr to resolve any issues with the ci test script if any?

awni commented 4 months ago

Good idea - and yea I think I can push this as a branch to check that

awni commented 4 months ago

CI is running on this branch https://github.com/ml-explore/mlx/tree/extension_ci

awni commented 4 months ago

@dc-dc-dc indeed the build did not work. Can you view this file ?

dc-dc-dc commented 4 months ago

Yeah, seems like it’s an issue when installing using a virtual env. when installing globally it’s fine, might be that the install location in make doesn’t line up, will look into this

dc-dc-dc commented 4 months ago

@awni can you run it again? I suspect it may be installing a version of mlx on pypi(without the fix) and causing the error. I set it to install from my fix branch for now to test

awni commented 4 months ago

I will run it, but I see two options to move forward

  1. Remove the CI change and put it in a follow up PR (and pin it to a weekly distribution / the next release)
  2. Build mlx locally rather than from PyPi

Wdyt?

dc-dc-dc commented 4 months ago

Will update the pyproject.toml to point to mlx-explore/mlx/main as after the merge it should work. Then after the next release can point it back to pypi with a version requirement.

For now can omit from the CI tests and add it back when the extension api gets in a more defined state, like you mentioned its still experimental so don't want to enforce that its tests pass as it can change.

awni commented 4 months ago

Thanks!!