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

Allow for overriding device, return device setup #210

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

ArthurBrussee
Copy link
Contributor

It'd be nice to allow for automatically overriding the wgpu device cube uses when using WgpuDevice::BestAvailable.

This PR adds a CUBECL_WGPU_DEVICE env var that be overriden. The names are the same as the enum - just implemented manually for now, I don't think it's worth pulling in more complex enum parsing machinery.

Additionally, this PR:

  • Returns a "WgpuSetup" from init_(a)sync, to allow people to intercept what devices are actually being used
  • Uses wpgu logic to select the highest performing device
  • Remove some leftover code in the wpu-spirv server that also created devices. Only C::request_device needs to be different between these two backends.

crates/cubecl-wgpu/src/runtime.rs Show resolved Hide resolved
crates/cubecl-wgpu/src/runtime.rs Outdated Show resolved Hide resolved
Comment on lines 159 to 160
pub async fn create_wgpu_setup<G: GraphicsApi, C: WgpuCompiler>(device: &WgpuDevice) -> WgpuSetup {
let (instance, adapter) = select_adapter::<G>(device).await;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah it's confusing with the functions init_async and init_sync. Maybe instead of init we could have register_setup_sync and register_setup_async. Also we could remove wgpu from the function name here, since it's in the wgpu project: create_setup is probably sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this could do with better naming. Register didn't feel right. This function does create new things, not just sign them up somewhere. Initialization is better imo (or maybe create). I went for this in the end:

  • init_sync -> init_device_sync.
  • init -> init_device
  • init_existing_device -> init_device_on_setup
  • create_client -> create_client_on_setup, and is now private, doesn't need to be public
  • create_wgpu_setup -> create_setup_for_device, and is now private, doesn't need to be public

Lmk how that looks.

crates/cubecl-wgpu/src/runtime.rs Outdated Show resolved Hide resolved
@nathanielsimard nathanielsimard merged commit 0481ca3 into tracel-ai:main Oct 29, 2024
5 checks passed
@ArthurBrussee ArthurBrussee deleted the custom-device branch October 29, 2024 19:27

/// Create a `WgpuDevice` on an existing `WgpuSetup`. Useful when you want to share
/// a device between CubeCL and other wgpu libraries.
pub fn init_device_on_setup(setup: WgpuSetup, options: RuntimeOptions) -> WgpuDevice {
Copy link
Contributor

@AsherJingkongChen AsherJingkongChen Oct 29, 2024

Choose a reason for hiding this comment

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

I am confused, are they all creating and registering devices?

  • fn init_device_on_setup(WgpuSetup) -> WgpuDevice
  • async fn init_device(WgpuDevice) -> WgpuSetup
  • fn init_device_async(WgpuDevice) -> WgpuSetup
  • async fn create_setup_for_device(WgpuDevice) -> WgpuSetup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Init_device and init_device_async are for initializing a cube WgpuDevice, and create all the wgpu resources.

init_device_on_setup initializes a cube WgpuDevice given existing wgpu resources.

Sorry if they are a bit confusing, maybe other names could be better!

Copy link
Contributor

@AsherJingkongChen AsherJingkongChen Oct 29, 2024

Choose a reason for hiding this comment

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

I have figured out their usage. However, I revised function names based on suggestions from nathanielsimard in #214.

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.

3 participants