Nvtx transform #2713

drbeh · 2021-08-06T21:34:01Z

Fixes #2712

Description

This PR implements transformation for NVIDIA Tool Extension (NVTX) functions.

Status

Ready

Types of changes

Non-breaking change (fix or new feature that would not break existing functionality).
New tests added to cover the changes.
Quick tests passed locally by running ./runtests.sh --quick --unittests.
In-line docstrings updated.
Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>

drbeh · 2021-08-06T21:51:39Z

Hi @wyli and @Nic-Ma,
These transformations only have effect when they are run with nsys profile, and also the results need to be visualized by "Nsight Systems" in order to be checked, so I am not sure how to write a test case to work with MONAI CI/CD. I have tested them in my local machine, and they are working as expected. What do suggest to do?
Considering that it does not have any effect on the rest of MONAI components, is the local testing fine?

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

…ansform

drbeh · 2021-08-10T00:40:38Z

@wyli and @Nic-Ma, I added some unittests to make sure that these tranforms do not alter anything when they are used alone or in conjunction with other array or dictionary transforms.

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Nic-Ma

Hi @drbeh ,

Thanks for bringing this interesting feature to MONAI.
To verify and test the APIs, I would suggest you to add a simple tutorial notebook in the MONAI tutorials repo, and put the link in the doc-string of this module. Then users can easily get start to use it and we can also easily verify & test it.

Thanks.

monai/transforms/nvtx.py

tests/test_nvtx_transform.py

Nic-Ma · 2021-08-10T03:01:19Z

@ericspod @rijobro @wyli ,

Do you guys have any other comments?

Thanks in advance.

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

drbeh · 2021-08-10T16:41:09Z

@ericspod @rijobro @wyli ,

Do you guys have any other comments?

Thanks in advance.

I'd appreciate it if you can give me any comment that you have.
Thanks.

wyli · 2021-08-10T18:34:33Z

Thanks for the PR, my feeling is that the NVTX tools are not well-known in our community. Shall we have some tutorials first, then revisit this PR and move the reusable code blocks from the tutorials into the core codebase? I don't know why these are wrapped as transforms instead of network layers, but I guess tutorials would clarify this easily.

drbeh · 2021-08-10T19:10:49Z

Thanks for the PR, my feeling is that the NVTX tools are not well-known in our community. Shall we have some tutorials first, then revisit this PR and move the reusable code blocks from the tutorials into the core codebase? I don't know why these are wrapped as transforms instead of network layers, but I guess tutorials would clarify this easily.

@wyli, tutorial would be great to teach the community on how to use Nsight System and NVTX.
Nsight is able to profile the network by default, so there is no need to wrap them as network. To profile a chain of transforms, you should either put NVTX in MONAI source code for each transform, or wrap them as transforms to be able to put them where is need in a transform pipeline. This PR is doing the latter, which is not intrusive and does not affect any other components in MONAI.

Nic-Ma · 2021-08-10T22:34:25Z

Hi @drbeh ,

Thanks for bringing this interesting feature to MONAI.
To verify and test the APIs, I would suggest you to add a simple tutorial notebook in the MONAI tutorials repo, and put the link in the doc-string of this module. Then users can easily get start to use it and we can also easily verify & test it.

Thanks.

Hi @drbeh ,

As I said in previous comment, I agree with @wyli , we should make a tutorial first and put the link in the doc-string of this module.

Thanks.

drbeh · 2021-08-10T23:34:35Z

Hi @drbeh ,

As I said in previous comment, I agree with @wyli , we should make a tutorial first and put the link in the doc-string of this module.

Thanks.

@wyli, @Nic-Ma, I definitely agree with having a tutorial on this but the tutorial will depend on these components, so shouldn't we first merge this and then use this in a tutorial?!

Similar to the other components where we first have implemented in MONAI (like other transforms and models), and then created tutorial for spleen segmentation for example, right?

Nic-Ma · 2021-08-10T23:36:49Z

Hi @drbeh ,

I would suggest you to develop a tutorial notebook PR in the tutorials repo, then let's merge these 2 PRs together.

Thanks.

drbeh · 2021-08-10T23:37:40Z

Hi @drbeh ,

I would suggest you to develop a tutorial notebook in the tutorials repo, then let's merge these 2 PRs together.

Thanks.

Sure, I can do that.

drbeh · 2021-08-11T01:32:20Z

Hi @Nic-Ma , @wyli

I have created a notebook and necessary codes for profiling digital pathology transformation pipeline using these NVTX transforms, and NVIDIA Nsight System. Please check it out: Project-MONAI/tutorials#305

Thanks

madil90 · 2021-08-11T06:04:38Z

/build

Nic-Ma

Thanks for the tutorial, looks good to me now.

wyli · 2021-08-12T08:26:15Z

monai/transforms/nvtx.py

+ """
+
+
+class RangePop(Transform):


according to the tutorial, I think those push/pop operations should be implemented as a python context manager for better readability, what do you think?

@wyli, you're right but the reason that we have separate push/pop transforms is that the range that they define is not limited to a transform and can include multiple transforms. Or even a push (start of a range) can be within transform chain and pop (end of range) somewhere else.

I'll implement the context manager too. #2754

drbeh added 4 commits August 6, 2021 21:03

Add NVTX transforms

6d46c3f

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>

Update transforms init

12c2723

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>

Update docs

c607b1a

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>

Sort imports

42c91c5

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>

drbeh added 2 commits August 7, 2021 10:43

Merge branch 'dev' into nvtx_transform

a4ae30e

Merge branch 'dev' into nvtx_transform

abe6903

drbeh requested review from Nic-Ma and wyli August 9, 2021 14:00

Merge branch 'dev' into nvtx_transform

5fd35d3

drbeh linked an issue Aug 9, 2021 that may be closed by this pull request

Performance Analysis of Digital Pathology Pipeline (ETA 8/27) #2728

Closed

drbeh removed a link to an issue Aug 9, 2021

Performance Analysis of Digital Pathology Pipeline (ETA 8/27) #2728

Closed

drbeh added 3 commits August 10, 2021 00:24

Update randomized transforms

8ba4f37

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Add unittests

20a76a8

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Merge branch 'nvtx_transform' of github.com:behxyz/MONAI into nvtx_tr…

1f10acb

…ansform

drbeh added 4 commits August 10, 2021 00:43

sort imports

09b2d46

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Add dictionary-based variants

b0c6020

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Fix parametrized and skip order

97c26e4

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Sort import

c6cd85b

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Nic-Ma reviewed Aug 10, 2021

View reviewed changes

Nic-Ma requested review from ericspod and rijobro August 10, 2021 03:00

drbeh added 3 commits August 10, 2021 13:43

Update rand tranforms

e7a8118

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Update test cases

05bd0b6

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Update test cases

3a2476c

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Merge branch 'dev' into nvtx_transform

d02e895

Merge branch 'dev' into nvtx_transform

a49c1c6

Nic-Ma mentioned this pull request Aug 12, 2021

Pathology Pipeline Profiling (NVTX and Nsight) Project-MONAI/tutorials#305

Merged

1 task

Nic-Ma approved these changes Aug 12, 2021

View reviewed changes

Merge branch 'dev' into nvtx_transform

9d4baa9

drbeh enabled auto-merge (squash) August 12, 2021 03:58

drbeh disabled auto-merge August 12, 2021 03:58

drbeh enabled auto-merge (squash) August 12, 2021 03:58

wyli reviewed Aug 12, 2021

View reviewed changes

drbeh mentioned this pull request Aug 12, 2021

NVTX range push/pop transform #2754

Closed

drbeh merged commit 8726dd5 into Project-MONAI:dev Aug 12, 2021

drbeh deleted the nvtx_transform branch August 12, 2021 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nvtx transform #2713

Nvtx transform #2713

drbeh commented Aug 6, 2021 •

edited

Loading

drbeh commented Aug 6, 2021 •

edited

Loading

drbeh commented Aug 10, 2021 •

edited

Loading

Nic-Ma left a comment

Nic-Ma commented Aug 10, 2021

drbeh commented Aug 10, 2021

wyli commented Aug 10, 2021

drbeh commented Aug 10, 2021

Nic-Ma commented Aug 10, 2021

drbeh commented Aug 10, 2021 •

edited

Loading

Nic-Ma commented Aug 10, 2021 •

edited

Loading

drbeh commented Aug 10, 2021

drbeh commented Aug 11, 2021

madil90 commented Aug 11, 2021

Nic-Ma left a comment

wyli Aug 12, 2021

drbeh Aug 12, 2021 •

edited

Loading

drbeh Aug 12, 2021

Nvtx transform #2713

Nvtx transform #2713

Conversation

drbeh commented Aug 6, 2021 • edited Loading

Description

Status

Types of changes

drbeh commented Aug 6, 2021 • edited Loading

drbeh commented Aug 10, 2021 • edited Loading

Nic-Ma left a comment

Choose a reason for hiding this comment

Nic-Ma commented Aug 10, 2021

drbeh commented Aug 10, 2021

wyli commented Aug 10, 2021

drbeh commented Aug 10, 2021

Nic-Ma commented Aug 10, 2021

drbeh commented Aug 10, 2021 • edited Loading

Nic-Ma commented Aug 10, 2021 • edited Loading

drbeh commented Aug 10, 2021

drbeh commented Aug 11, 2021

madil90 commented Aug 11, 2021

Nic-Ma left a comment

Choose a reason for hiding this comment

wyli Aug 12, 2021

Choose a reason for hiding this comment

drbeh Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

drbeh Aug 12, 2021

Choose a reason for hiding this comment

drbeh commented Aug 6, 2021 •

edited

Loading

drbeh commented Aug 6, 2021 •

edited

Loading

drbeh commented Aug 10, 2021 •

edited

Loading

drbeh commented Aug 10, 2021 •

edited

Loading

Nic-Ma commented Aug 10, 2021 •

edited

Loading

drbeh Aug 12, 2021 •

edited

Loading