Add the options data class to program #237
Conversation
leofang
added
P1
Thanks, Berry 🙏 We welcome feedbacks and won't ignore anyone! |
|
Another TODO: Let's also update the code samples and show the best practice! |
Is there a way to add these as doctests so that the code samples must always be kept in working order? |
|
@warsawnv Totally. I'm actively working on getting github actions working, and the examples are invoked as part of the testsuite. |
|
/ok to test |
|
Very good point |
ksimpson-work
force-pushed
the
ksimpson/add_program_options
branch
from
03128c8 to
6d789cb
Compare
|
/ok to test |
There was a problem hiding this comment.
Thanks a lot, Keenan! Very comprehensive changes, appreciate the hard work! We are getting there!
I left a final batch of comments, in two categories:
- Formatting related to Sphinx: This may require another local sweep to clean up; my suggestions can't cover all files that are touched
- Actual code/behavior change: Only a few places. See my comments below.
| if not culink_backend: | ||
| from cuda.bindings import nvjitlink |
There was a problem hiding this comment.
I may have missed something: Why would culink_backend be None? Isn't it using either nvJitLink or driver API?
There was a problem hiding this comment.
I am using it as a boolean flag. So it could be false, but not None in this case.
| object_code_a_ptx = Program(kernel_a, "c++").compile("ptx", options=("-rdc=true",)) | ||
| object_code_b_ptx = Program(device_function_b, "c++").compile("ptx", options=("-rdc=true",)) | ||
| object_code_c_ptx = Program(device_function_c, "c++").compile("ptx", options=("-rdc=true",)) | ||
| object_code_a_ptx = Program(kernel_a, "c++", ProgramOptions(relocatable_device_code=True)).compile("ptx") |
There was a problem hiding this comment.
FYI, in case you find it more pleasant than the present form: Our dataclass options can also be initialized by Python dict (by design), so this should also work:
| object_code_a_ptx = Program(kernel_a, "c++", ProgramOptions(relocatable_device_code=True)).compile("ptx") | |
| object_code_a_ptx = Program(kernel_a, "c++", {"relocatable_device_code": True}).compile("ptx") |
(not saying we should do it here, I'll leave this up to you to decide)
There was a problem hiding this comment.
gotcha. I personally like the non-dictionary version, but thanks for letting me know. I did not know that was a possibility
Co-authored-by: Leo Fang <leof@nvidia.com>
Co-authored-by: Leo Fang <leof@nvidia.com>
Co-authored-by: Leo Fang <leof@nvidia.com>
…to ksimpson/add_program_options
Co-authored-by: Leo Fang <leof@nvidia.com>
…to ksimpson/add_program_options
|
/ok to test |
…to ksimpson/add_program_options
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
last few comments addressed, tests improved and passing |
|
I still see many review comments above not addressed? |
|
one was an optional suggestion and one was a question. I didn't want to makr as resolved so you had a chance to see my response, but there weren't associated code changes |
close #221
Add the options class to Program.
This comes with a few side effects, namely: There are some modifications to the tests, and the LinkerOptions class now accepts None for arch (using the current device as a default), for consistency between the program options and linker options