Skip to content
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

Automatically fetch and register PjRT Metal plugin #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mofeing
Copy link
Collaborator

@mofeing mofeing commented Sep 4, 2024

  • Move artifact to a new PjRT_Metal_jll package
  •  Try local build

@wsmoses is there a Metal infra available for testing?

@mofeing mofeing requested a review from wsmoses September 4, 2024 20:01
@mofeing mofeing added the enhancement New feature or request label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Benchmark Results

main 0980193... main/098019337b01a7...
comptime/basics/2D sum 27.8 ± 0.78 ms 27.3 ± 0.91 ms 1.02
comptime/basics/Basic cos 26 ± 0.83 ms 26 ± 0.59 ms 0.999
comptime/basics/Basic grad cos 0.045 ± 0.0012 s 0.045 ± 0.00082 s 1
time_to_load 1.29 ± 0.039 s 1.3 ± 0.0042 s 0.987

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@wsmoses
Copy link
Member

wsmoses commented Sep 4, 2024

I think just macos CI should do it, no?

@@ -183,6 +183,32 @@ extern "C" PjRtClient* MakeTPUClient(const char* tpu_path , const char** error)
return GetCApiClient("TPU");
}

const char* const kEnvMetalLibraryPath = "METAL_LIBRARY_PATH";

extern "C" PjRtClient* MakeMetalClient(const char* libpath, const char** error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid making a separate api change atm since the oher loadpjrt plugins have an api presently?

Just for the sake of setting it up without a jll bump

@avik-pal
Copy link
Collaborator

avik-pal commented Sep 5, 2024

I think just macos CI should do it, no?

We might want to use the buildkite ones here. The macos-large GH runners have GPUs but those are paid. The free ones don't have GPU.

Buildkite script for that https://github.com/LuxDL/MLDataDevices.jl/blob/b7e05516c0c0ec554259d90b5d620b6d0bf12154/.buildkite/testing.yml#L110-L134.

@wsmoses
Copy link
Member

wsmoses commented Sep 5, 2024 via email


try
metal = MetalClient(metaldir * "/pjrt_plugin_metal_14.dylib")
backends["metal"] = metal
Copy link
Member

Choose a reason for hiding this comment

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

We might as well just label this as gpu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants