Skip to content

Conversation

@bhashemian
Copy link
Member

@bhashemian bhashemian commented Aug 6, 2021

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>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@bhashemian
Copy link
Member Author

bhashemian commented Aug 6, 2021

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?

@bhashemian bhashemian requested review from Nic-Ma and wyli August 9, 2021 14:00
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
@bhashemian
Copy link
Member Author

bhashemian commented Aug 10, 2021

@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>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

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.

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

Nic-Ma commented Aug 10, 2021

@ericspod @rijobro @wyli ,

Do you guys have any other comments?

Thanks in advance.

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
@bhashemian
Copy link
Member Author

@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
Copy link
Contributor

wyli commented Aug 10, 2021

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.

@bhashemian
Copy link
Member Author

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
Copy link
Contributor

Nic-Ma commented Aug 10, 2021

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.

@bhashemian
Copy link
Member Author

bhashemian commented Aug 10, 2021

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
Copy link
Contributor

Nic-Ma commented Aug 10, 2021

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.

@bhashemian
Copy link
Member Author

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.

@bhashemian
Copy link
Member Author

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
Copy link
Contributor

madil90 commented Aug 11, 2021

/build

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Thanks for the tutorial, looks good to me now.

@bhashemian bhashemian enabled auto-merge (squash) August 12, 2021 03:58
@bhashemian bhashemian disabled auto-merge August 12, 2021 03:58
@bhashemian bhashemian enabled auto-merge (squash) August 12, 2021 03:58
"""


class RangePop(Transform):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@bhashemian bhashemian Aug 12, 2021

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll implement the context manager too. #2754

@bhashemian bhashemian merged commit 8726dd5 into Project-MONAI:dev Aug 12, 2021
@bhashemian bhashemian deleted the nvtx_transform branch August 12, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NVTX in MONAI

4 participants