chore(devicePicker): handle more and return device type#113
chore(devicePicker): handle more and return device type#113Luthaf merged 5 commits intometatensor:mainfrom
Conversation
HaoZeke
force-pushed
the
tdevpicker
branch
3 times, most recently
from
77c7978 to
b23efc2
Compare
PicoCentauri
left a comment
PicoCentauri
left a comment
There was a problem hiding this comment.
Thanks! I am fine with having a different API for C and Python.
Maybe we also ask for a comment by @Luthaf.
|
I'm happy to add support for all other device types in the function checking for availability, but I don't see a point returning a DeviceType instead of a string. This would be a breaking change requiring new releases of everything that depends on metatomic, and downstream code can always create the torch Device themself. |
PicoCentauri
left a comment
PicoCentauri
left a comment
There was a problem hiding this comment.
Nice. Tests are not happy though :-(
HaoZeke
force-pushed
the
tdevpicker
branch
4 times, most recently
from
e0f39ea to
d444ce1
Compare
Luthaf
left a comment
Luthaf
left a comment
There was a problem hiding this comment.
What's the plan for backward compatibility? This is only breaking the C++ API, which should only be used in LAMMPS.
|
Yes, I will fix it asap in lammps. It should also not break lammps as the version there is pinned |
PLUMED/EON/GROMACS? (too much screamcase) |
|
PLUMED not using it |
HaoZeke
force-pushed
the
tdevpicker
branch
2 times, most recently
from
c821e13 to
9d4126f
Compare
Refactors the C++ function to perform more robust device detection and normalization. - Changes the return type from std::string to c10::DeviceType. - Adds helper functions for string normalization, device availability checks (is_known_device, available_device), and mapping to the DeviceType enum. - Adds warnings for unknown or unavailable devices. - Throws an error on fallback instead of defaulting to CPU.
Moves the Python binding logic for out of a complex lambda and into a standalone function. - This wrapper handles the conversion from the C++ c10::DeviceType to the Python-facing string. - It strips the device index (e.g., 'cpu:0' -> 'cpu') for backwards compatibility. - Includes robust try/catch blocks to convert c10::Error into std::runtime_error for Python.
Luthaf
left a comment
Luthaf
left a comment
There was a problem hiding this comment.
One final suggestion and then we can merge!
Co-authored-by: Guillaume Fraux <luthaf@luthaf.fr>
- Updates C++ tests in 'misc.cpp' to align with the new c10::DeviceType return value. - Adds a new Python test suite 'test_pick_device.py'. - Tests basic selection, requested device selection, error handling for unavailable devices, and warnings for unrecognized devices.
|
Will merge when green ^_^ |
3 participants
I really like #90 however I realized in TheochemUI/eOn#262 that it might be more ergonomic to pass around the
devicetypeinstead of the string representation. The Python side should still get thestrof course.Plus this adds the rest of the devices supported by Torch, except the
privateusething which sounds kinda scammy.Contributor (creator of pull-request) checklist
Reviewer checklist