Upstream RAPIDS-Triton with name changes #7
Conversation
|
@GuanLuo Shouldn't we change the licenses to the way they are in Triton? Or should we keep it Apache? |
| /* inline auto* get_memory_resource(device_id_t device_id) | ||
| { | ||
| auto rmm_device_id = rmm::cuda_device_id{device_id}; | ||
| return rmm::mr::get_per_device_resource(rmm_device_id); | ||
| } | ||
|
|
||
| inline auto* get_memory_resource() { return rmm::mr::get_current_device_resource(); } */ |
There was a problem hiding this comment.
Just curious, what is this commented out for? Is it needed?
There was a problem hiding this comment.
It's from the original code base. Probably just some artifacts.
| /* Triton expects certain definitions within its backend libraries to follow | ||
| * specific naming conventions. Specifically, for a backend named | ||
| * "tools_identity," most definitions should appear within a namespace called | ||
| * triton::backend::tools_identity. | ||
| * | ||
| * In order to facilitate this with minimal effort on the part of backend | ||
| * developers, we ask that you put the name of your backend here. This macro is | ||
| * then used to propagate the correct namespace name wherever it is needed in | ||
| * the impl and interface code. */ | ||
|
|
||
| #define NAMESPACE tools_identity |
There was a problem hiding this comment.
- I notice we don't have a README for the backend folder. Can we add one? (Can be a separate ticket) -- maybe we can borrow mostly from this one for now.
- Assuming we have a README, this
NAMESPACEconcept seems like something we should document there for users.
There was a problem hiding this comment.
Let me take another look at the namespacing, I think it is different from the original design in rapids-triton noew because we replace it with triton::developer_tools, the name lookup is not direct and I did modify some places for proper looking (explicitly added triton::backend::)
There was a problem hiding this comment.
Let's hold on on this (and this PR generally). We have extensive docs for the backend wrappers, and we need to figure out where they will live in relation to this repo. Most importantly, we'll need to figure out where the backend template repo will live, since it needs to be a truly separate GH repo.
In terms of this specific issue, yes, we still need this NAMESPACE technique, but its use is documented elsewhere. This is for generating a specific backend using the wrappers; it is not a namespace that appears generally in the wrappers themselves.
|
Following some offline discussion, we probably want to hold off on merging this PR just yet. I'll set up the preliminary infrastructure we discussed, and then we can get these changes in once we have the rest of the foundation in place. |
There was a problem hiding this comment.
Putting in a "request changes" placeholder review mostly to ensure that we don't merge these changes prematurely. Among other things, this PR breaks existing CI functionality and CPM inclusion, both of which will need to be addressed. I've started #13 to restore that functionality and add in the other pieces we've discussed, and then we can merge some of the renaming stuff from here to there. That PR is still very much a draft though.
@wphicks this PR is source from main branch of rapids-triton repo, I removed the
squeeze_outputas it seems to be just for backward compatibility. Other than that, all changes are name changes:namespace rapids -> namespace toolsRapids_Triton -> Backend_ToolsRapids -> Tools