Remove GraphicsAPI generic for WgpuRuntime #1888
Conversation
ArthurBrussee
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1888 +/- ##
==========================================
- Coverage 86.14% 86.07% -0.07%
==========================================
Files 778 781 +3
Lines 90827 91615 +788
==========================================
+ Hits 78245 78862 +617
- Misses 12582 12753 +171 ☔ View full report in Codecov by Sentry. |
| let (adapter, device_wgpu, queue) = | ||
| pollster::block_on(create_wgpu_setup::<AutoGraphicsApi>(device)); |
There was a problem hiding this comment.
So if I understand it well, this is where the default runtime is actually created for a device type. It uses the AutoGraphicsApi to select the most appropriate adaptor for the device. If you want to specify manually the graphics API for a device type, you have to use init_sync or init_async!
I don't think this is a bad thing, now the WgpuBackend only has typical backend generics, and runtime options can be changed using the init functions! Before merging I would add a bit more documentation on the WgpuRuntime and WgpuBackend pointing to the right direction to specify backend and runtime options.
There was a problem hiding this comment.
Yep that's right! Seemed coherent to me to have one "advanced initialize" place, glad you agree :)
Have added some docs now, and a random code cleanup, lmk how this looks!
Checklist
run-checks allscript has been executed.Changes
Slightly cheeky PR so bear with me :)
Ran into some annoying trait bounds while trying to do some stuff and rather than just grumble, felt like I could remove this entirely! A WgpuRuntime is a WgpuRuntime - irregardless of what the graphics API is. This is particularly evident with
runtime::init_existing_device()- it's possible to register a DX12 device, and yet run it on a BurnRuntime which just seems wrong!I understand it's used to manually pick a graphics API, but, you can already do this with
This could also be in RuntimeOptions perhaps, for consistency.
All that said, this is a bit of miscellaneous design change in a PR so thanks for bearing with me :)