-
Notifications
You must be signed in to change notification settings - Fork 46
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
enable directml #356
enable directml #356
Conversation
4963ba4
to
a8fe38f
Compare
@tazlin here I'd like a thorough review. I got it to a state where it works locally, but only if I set The I'll write some user documentation when I know the core logic is actually working as best it can. As for the commit history: I can clean that up before the merge, everything up to |
Torch versions, especially experimental plugins, are notorious for introducing breaking changes version to version. Pinning will help minimize this chance.
With the introduction of directml and the current landscape, its likely we may yet increase the number of supported driving technologies, so I've refactored the relevant tests to be more modular
I've finally gotten around to doing some more stress testing and writing the necessary docs. The main @tazlin this should be ready to merge. This would flow very naturally with the main Small sidenote: Support for Docker on Windows was planned, but isn't implemented yet, I've removed that mention from the AMD on Windows section for now. |
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.
Runs as expected (though at the documented severely reduced inference speed compared to CUDA), code changes are not expected to impact any other use case at this time as they are narrow in scope.
I do sense that the increased number of requirements.x.txt
files for each backend is going to be an issue at some point, but with the testing in place to check for consistency, I'm satisfied with the approached until a better one can be identified (perhaps using uv
)
This should broaden our compatibility, although with significantly worse performance than native libraries.
ToDo:
Worse Performance shold be documented and advised against, before announcing this.
The PR still needs the knobs and dials to only enable directml under certain conditions, easiest would be an eqivalent to
--amd
and then maybe another initialization script?Or maybe think about a way to properly detect what plattform we are running on and go from there?
Having 4 different
update-runtime
scripts mostly doing the same is getting a bit out of hand...