-
Notifications
You must be signed in to change notification settings - Fork 122
update: unlock native Linux build for CMake and CPU backend only #293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
update: unlock native Linux build for CMake and CPU backend only #293
Conversation
Signed-off-by: Melissa Kilby <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
|
|
||
| # todo: add conditional logic for MLX_BUILD_CUDA (once implemented) | ||
| if(NOT MLX_BUILD_METAL) | ||
| list(REMOVE_ITEM MLX-src ${CMAKE_CURRENT_LIST_DIR}/Source/MLX/GPU.swift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the API in here has moved to memory.h and is no longer GPU specific:
int mlx_clear_cache(void);
int mlx_get_active_memory(size_t* res);
int mlx_get_cache_memory(size_t* res);
int mlx_get_memory_limit(size_t* res);
int mlx_get_peak_memory(size_t* res);
int mlx_reset_peak_memory(void);
int mlx_set_cache_limit(size_t* res, size_t limit);
int mlx_set_memory_limit(size_t* res, size_t limit);
int mlx_set_wired_limit(size_t* res, size_t limit);
we don't need to deal with it here but perhaps this needs some refactoring -- we can forward the "GPU" methods to these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought down the road it could be nice to have GPU.swift (cross-platform) and GPU+Metal.swift (only Apple Silicon builds) files and later a GPU+CUDA.swift (cross-platform) file ... wasn't quite yet too sure how to split GPU.swift. If you believe it's ok to add into this PR, happy to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be done here or left until later. I was actually thinking a Memory.swift like this:
Those are actually functions and properties currently on GPU but they have moved on the python side, so I think we would want to deprecate current ones and forward to the (moved) API.
I don't know if we would want a Metal.swift as it would collide with the framework. So yes, perhaps GPU+X and keep the GPU specific parts under the GPU type.
Anyway, I don't think this is critical and we could pick it up later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look and let you know (I am just starting out to learn this code base). Plus we need new CI for sure -- sorry forgot about this for a second.
| arrayBasics() | ||
| automaticDifferentiation() | ||
| let osName = ProcessInfo.processInfo.operatingSystemVersionString.lowercased() | ||
| let device: Device = osName.contains("linux") ? .cpu : .gpu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need to do something inside the default properties along these lines so that the calling process would get the default for the platform? Though I suppose this might change once this build supports CUDA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and I like your suggestion to just wait this one out and see how the CUDA integration turns out to be for Linux.
this is from the |
Co-authored-by: David Koski <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>
|
I wonder if it is worth adding a circleci config to do the linux build? |
Let's do GitHub workflows -- let me check on this. We should have one YUM based and one DEB based distro to get started. |
True, I guess I should get that set up. Right now CI tests:
Compile is probably the most important as we could very easily break the linux build. The unit test one has a hook |
|
Quick note: New plan see #294 meaning let's wait before merging this PR, but it should be considered complete in scope. |
Unlock native Linux build for CMake and CPU backend only.
Related to #258.
This PR follows the principle of making the minimal initial changes necessary to unlock native Linux builds with CMake —nothing more.
The CUDA backend for native Linux builds in CMake is something to explore afterward, knowing there are already things moving (see #258).
CC @davidkoski and @zamderax
@Joannis for follow (SSWG)