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

Move definitions to cpp #4124

Merged
merged 1 commit into from
Mar 21, 2025
Merged

Move definitions to cpp #4124

merged 1 commit into from
Mar 21, 2025

Conversation

wujingyue
Copy link
Collaborator

tests/cpp/utils.h is a busy header file used by all tests.

tests/cpp/utils.h is a busy header file used by all tests.
@wujingyue wujingyue requested a review from zasdfgbnm March 21, 2025 13:30
@wujingyue
Copy link
Collaborator Author

!test

Copy link

Description

  • Moved NVFuserTest methods to utils.cpp

  • Updated utils.h to declare methods

  • Added GTest::gtest to benchmark targets


Changes walkthrough 📝

Relevant files
Enhancement
utils.cpp
Added NVFuserTest method definitions                                         

tests/cpp/utils.cpp

  • Added definitions for SetUp, TearDown, captureStdout,
    ensureStopCaptureStdout, and getCapturedStdout methods of NVFuserTest
    class.
  • +64/-0   
    utils.h
    Declared NVFuserTest methods                                                         

    tests/cpp/utils.h

  • Declared SetUp, TearDown, captureStdout, ensureStopCaptureStdout, and
    getCapturedStdout methods of NVFuserTest class.
  • +5/-63   
    Configuration changes
    CMakeLists.txt
    Added GTest::gtest to benchmark targets                                   

    CMakeLists.txt

  • Added GTest::gtest to target_link_libraries for nvfuser_bench and
    nvfuser_multidevice_bench.
  • +2/-0     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Logging Initialization

    Ensure that c10::initLogging(); is the correct approach for initializing logging in this context. Verify if there are any side effects or performance implications.

    c10::initLogging();
    Random Seed Handling

    Verify that the random seed handling logic is consistent with the rest of the codebase and that it correctly handles both provided and default seeds.

    at::manual_seed(getATenRandomSeed());
    
    // If NVFUSER_TEST_ATEN_RANDOM_SEED is provided, use that for the ATen
    // random seed. Otherwise, use zero. If a test fails, this seed will be
    // printed.
    std::srand(getCRandomSeed());
    
    GTest Dependency

    Ensure that adding GTest::gtest as a dependency is necessary and does not introduce any conflicts or unnecessary dependencies.

    GTest::gtest
    

    @@ -806,6 +806,7 @@ if(BUILD_NVFUSER_BENCHMARK)
    )
    target_include_directories(nvfuser_bench PUBLIC ${NVFUSER_ROOT})
    target_link_libraries(nvfuser_bench PRIVATE
    GTest::gtest
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    Including tests/cpp/utils.cpp in source files without linking gtest was a bug. This change fixes it.

    @wujingyue wujingyue merged commit dd0f6a5 into main Mar 21, 2025
    52 of 53 checks passed
    @wujingyue wujingyue deleted the wjy/util branch March 21, 2025 19:09
    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.

    2 participants