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.

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

Use nvtx3 to avoid deprecation warning #3872

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Use nvtx3 to avoid deprecation warning #3872

wants to merge 6 commits into from

Conversation

Priya2698
Copy link
Collaborator

@Priya2698 Priya2698 commented Feb 11, 2025

Similar to #3712.

Gated by pytorch PR gated by pytorch/pytorch#147418

@Priya2698
Copy link
Collaborator Author

!build

Copy link

github-actions bot commented Feb 11, 2025

Review updated until commit eabc8c2

Description

  • Update nvtx to nvtx3 to avoid deprecation warnings

  • Modify build configuration to use system nvtx by default

  • Remove nvtx from RPATH in CMake

  • Add command-line option to disable system nvtx


Changes walkthrough 📝

Relevant files
Enhancement
transformer.cpp
Update `nvtx` include path                                                             

benchmarks/cpp/transformer.cpp

  • Updated include path for nvToolsExt.h to nvtx3/nvToolsExt.h
+1/-1     
Configuration changes
setup.py
Add system `nvtx` build configuration                                       

setup.py

  • Added BUILD_WITH_SYSTEM_NVTX flag set to True by default
  • Added --no-system-nvtx command-line option
  • +6/-0     
    CMakeLists.txt
    Remove `nvtx` from RPATH and link libraries                           

    CMakeLists.txt

  • Removed find_library for libnvToolsExt.so
  • Updated INSTALL_RPATH to remove nvtx/lib
  • Removed ${LIBNVTOOLSEXT} from target link libraries
  • +5/-9     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    Ensure that the inclusion of nvtx3/nvToolsExt.h does not break existing functionality and that nvtx3 is compatible with the current build environment.

    #include <nvtx3/nvToolsExt.h>
    Configuration

    Verify that the new BUILD_WITH_SYSTEM_NVTX flag is correctly handled and that it does not introduce any unintended side effects.

    BUILD_WITH_SYSTEM_NVTX = True
    Dependency Management

    Confirm that the removal of LIBNVTOOLSEXT and the addition of USE_SYSTEM_NVTX do not lead to missing dependencies or unresolved symbols during the build process.

    ${LIBCUPTI}
    

    @Priya2698 Priya2698 requested a review from wujingyue February 11, 2025 21:42
    @xwang233
    Copy link
    Collaborator

    find_library(LIBNVTOOLSEXT libnvToolsExt.so PATHS ${CUDA_TOOLKIT_ROOT_DIR}/lib64/)

    This line also needs to be upgraded. cc @nWEIdia

    @wujingyue wujingyue requested review from nWEIdia and removed request for wujingyue February 11, 2025 22:59
    @nWEIdia
    Copy link
    Collaborator

    nWEIdia commented Feb 11, 2025

    As the above review suggested: please focus on potential compilation breakages, e.g. how should the CMakeFile find nvtx3?

    @xwang233
    Copy link
    Collaborator

    !build

    @xwang233
    Copy link
    Collaborator

    !build

    @xwang233
    Copy link
    Collaborator

    !test

    @xwang233
    Copy link
    Collaborator

    !test

    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.

    3 participants