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 code from SetUp/TearDown to constructor/destructor #4130

Merged
merged 1 commit into from
Mar 22, 2025

Conversation

wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Mar 21, 2025

Copy link

Description

  • Moved setup code to constructor

  • Moved teardown code to destructor

  • Refactored NVFuserTest initialization


Changes walkthrough 📝

Relevant files
Enhancement
test_multidevice_overlap.cpp
Added teardown method to reset resources                                 

tests/cpp/test_multidevice_overlap.cpp

  • Added TearDown method to reset tc_locally_reduced_
  • Called OverlapTest::TearDown() in TearDown
  • +5/-0     
    utils.cpp
    Refactored setup and teardown to constructor and destructor

    tests/cpp/utils.cpp

  • Renamed SetUp to NVFuserTest constructor
  • Moved GPU check from SetUp to constructor
  • Renamed TearDown to NVFuserTest destructor
  • +9/-6     
    utils.h
    Updated NVFuserTest class declarations                                     

    tests/cpp/utils.h

  • Added constructor and destructor declarations
  • Removed TearDown method declaration
  • +2/-1     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Code Duplication

    The TearDown method is being overridden in CollectiveBasedOverlapTest but the original TearDown from OverlapTest is not being called. Ensure that the base class's TearDown is called to avoid missing any cleanup.

    void TearDown() override {
      tc_locally_reduced_.reset();
      OverlapTest::TearDown();
    }
    Code Duplication

    The SetUp method in NVFuserTest is being moved from the constructor to the SetUp method, but the original SetUp method still contains some initialization code that should be moved to the constructor. Ensure that all initialization code is in the constructor.

    void NVFuserTest::SetUp() {
      // requires PASCAL or newer
      if (!deviceMajorMinorCheck(6)) {
        GTEST_SKIP() << "skipping tests on pre-PASCAL GPUs";
      }
    }
    Missing Destructor Declaration

    The destructor ~NVFuserTest() is declared in the header file but not defined in the source file. Ensure that the destructor is properly defined.

    ~NVFuserTest() override;
    void SetUp() override;

    @wujingyue
    Copy link
    Collaborator Author

    !test

    @@ -195,6 +195,11 @@ class CollectiveBasedOverlapTest : public OverlapTest {
    tc_locally_reduced_ = at::empty(tc_locally_reduced_sizes, gpu_options_);
    }

    void TearDown() override {
    tc_locally_reduced_.reset();
    Copy link
    Collaborator Author

    @wujingyue wujingyue Mar 21, 2025

    Choose a reason for hiding this comment

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

    This is a leftover from another draft PR. It's not strictly required for this PR but in general things allocated in SetUp should be cleaned in TearDown. If it were allocated in the constructor, we could skip this because the destructor automatically destructs members.

    @wujingyue wujingyue requested a review from zasdfgbnm March 21, 2025 23:41
    @wujingyue wujingyue merged commit c63efa2 into main Mar 22, 2025
    38 of 39 checks passed
    @wujingyue wujingyue deleted the wjy/construct branch March 22, 2025 05:47
    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