Add nvtx annotatations to groupby methods #12941
Conversation
wence-
added
improvement
There was a problem hiding this comment.
@wence- This would be a good opportunity to add a section to our Developer Guide about what methods should be annotated. (Searching "nvtx" returns nothing right now.) Should (all/some) private methods be annotated, or only public methods? Let's look at past NVTX PRs for guidance here, like these:
We have been super inconsistent about this so far, but I think all (and only) public methods should be annotated. |
I'd vote for all public methods(& properties) to be annotated and select private methods on a case-by-case basis to be annotated. Historically we've annotated private methods for the sake of easier nvtx profiling while working on performance issues. |
…rapidsai#13188) (rapidsai#13195) Back ports the fix in rapidsai#13188 to fix v23.04 builds Authors: - Sevag H (https://github.com/sevagh) Approvers: - AJ Schmidt (https://github.com/ajschmidt8) - Vyas Ramasubramani (https://github.com/vyasr) - Bradley Dice (https://github.com/bdice)
There was a problem hiding this comment.
@wence- I updated this PR for 23.06. I think this is in a good state. Comments above mentioned that only "select" private methods should be annotated. I think the private methods that are annotated here are good candidates for annotation, so I would recommend merging this in the current state.
If we want to add documentation on when to add annotations, I think the discussion above is a good starting point but I would write that in a separate PR so this can merge without further delay.
|
/merge |
Description
Not sure if not annotating these was an oversight during implementation...
Checklist