Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Refactor nvtx annotations in cudf & dask-cudf #10396

Merged
merged 11 commits into from
Mar 9, 2022
Merged

[REVIEW] Refactor nvtx annotations in cudf & dask-cudf #10396

merged 11 commits into from
Mar 9, 2022

Conversation

galipremsagar
Copy link
Contributor

This PR consolidates all nvtx.annotate calls using common decorators cudf_nvtx_annotate & dask_cudf_nvtx_annotate that makes it easier to maintain and annotate APIs.

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer dask Dask issue labels Mar 7, 2022
@galipremsagar galipremsagar self-assigned this Mar 7, 2022
@galipremsagar galipremsagar requested review from a team as code owners March 7, 2022 21:52
@codecov
Copy link

codecov bot commented Mar 7, 2022 •
edited
Loading

Codecov Report

Merging #10396 (4c52101) into branch-22.04 (b3dc9d6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10396      +/-   ##
================================================
+ Coverage         86.12%   86.16%   +0.03%     
================================================
  Files               139      139              
  Lines             22450    22462      +12     
================================================
+ Hits              19336    19354      +18     
+ Misses             3114     3108       -6
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 93.56% <100.00%> (ø)
python/cudf/cudf/core/frame.py 91.73% <100.00%> (+<0.01%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.92% <100.00%> (+0.41%) ⬆️
python/cudf/cudf/core/index.py 92.19% <100.00%> (+<0.01%) ⬆️
python/cudf/cudf/core/indexed_frame.py 92.25% <100.00%> (ø)
python/cudf/cudf/core/multiindex.py 92.13% <100.00%> (ø)
python/cudf/cudf/core/series.py 95.16% <100.00%> (-0.01%) ⬇️
python/cudf/cudf/core/single_column_frame.py 97.03% <100.00%> (-0.03%) ⬇️
python/cudf/cudf/core/udf/utils.py 98.63% <100.00%> (ø)
python/cudf/cudf/io/csv.py 91.80% <100.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78eda63...4c52101. Read the comment docs.

@galipremsagar galipremsagar added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Mar 7, 2022
bdice
bdice reviewed Mar 8, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice to centralize this! I like the implementation. I have one comment below.

python/cudf/cudf/utils/utils.py Outdated Show resolved Hide resolved
@galipremsagar galipremsagar added non-breaking Non-breaking change and removed non-breaking Non-breaking change labels Mar 8, 2022
@galipremsagar galipremsagar changed the base branch from branch-22.04 to 21.12 March 8, 2022 19:32
@galipremsagar galipremsagar changed the base branch from 21.12 to branch-22.04 March 8, 2022 19:32
vyasr
vyasr approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, otherwise LGTM. Is there an issue to link this to, or is this PR just a response to the last request I made on your previous PR? Just want to make sure we prune issues if needed.

python/cudf/cudf/utils/utils.py
@@ -31,6 +34,7 @@
"__ge__",
}

_NVTX_COLORS = ["green", "blue", "purple", "rapids"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What color is "rapids"? Is it not very similar to purple?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those both aren't similar:
https://github.com/NVIDIA/NVTX/blob/eb0d6301e833fa3631d774272f09741582d29e94/python/nvtx/colors.py#L14-L15

@galipremsagar
Copy link
Contributor Author

One question, otherwise LGTM. Is there an issue to link this to, or is this PR just a response to the last request I made on your previous PR? Just want to make sure we prune issues if needed.

This was the one: #10374 (review)

@vyasr
Copy link
Contributor

vyasr commented Mar 9, 2022

One question, otherwise LGTM. Is there an issue to link this to, or is this PR just a response to the last request I made on your previous PR? Just want to make sure we prune issues if needed.

This was the one: #10374 (review)

Cool I was just verifying that there wasn't an issue that we needed to close.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Dask Reviewer labels Mar 9, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 70406de into rapidsai:branch-22.04 Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad left review comments

@bdice bdice bdice left review comments

@shwina shwina shwina left review comments

@vyasr vyasr vyasr approved these changes

@quasiben quasiben quasiben approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source skirui-source is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@galipremsagar galipremsagar

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge dask Dask issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@galipremsagar @vyasr @quasiben @shwina @bdice @jrhemstad