#13128: Enhance build_metal.sh #13141

blozano-tt · 2024-09-26T05:54:33Z

Ticket

Problem description

Currently, we build tt-metal, ttnn, and tt-umd unit tests, or we build no tests.
Additionally, our users have no control over where build artifacts are installed when building from source.

What's changed

Three new CMake options are defined
- TT_METAL_BUILD_TESTS -> Do we want to build tt_metal tests?
- TTNN_BUILD_TESTS -> Do we want to build ttnn tests?
- TT_UMD_BUILD_TESTS -> Do we want to build umd unit tests?
  - This option needs to be consumed by tt-umd, and would require a push to that repo, and submodule update.
CMAKE_INSTALL_PREFIX no longer tied to PROJECT_BINARY_DIR, give people freedom
Enhancements to build_metal.sh
- Allow customer to provide long options; i.e. --help
- Allow customer to provide installation directory, default to build_dir
- Allow customer to provide extra cmake options
- Add --clean option
- Add --debug, --release, --development as shortcuts to pick build type

Checklist

Post commit CI passes
Blackhole Post commit (if applicable)
Model regression CI testing passes (if applicable)
Device performance regression CI testing passes (if applicable)
New/Existing tests provide coverage for changes

tests/CMakeLists.txt

ttmchiou · 2024-09-27T17:26:00Z

"TT_UMD_BUILD_TESTS -> Do we want to build umd unit tests?
This option needs to be consumed by tt-umd, and would require a push to that repo, and submodule update."

Does this option need to be coordinated with UMD? if so, would suggest reaching out to @joelsmithTT @broskoTT @pjanevskiTT.

Is this a necessary change for getting rid of ARCH_NAME for tt-metal codebase?

blozano-tt · 2024-09-27T18:02:03Z

@ttmchiou

Does this option need to be coordinated with UMD? if so, would suggest reaching out to @joelsmithTT @broskoTT @pjanevskiTT.

Yes, I was going to submit a PR to them today. Will keep them in the loop.

Is this a necessary change for getting rid of ARCH_NAME for tt-metal codebase?
Yes, it is a very necessary stop gap, until the umd unit tests don't depend on ARCH_NAME, or until we no longer have ownership of running umd unit tests in our CI.

It is sort of complicated to talk through all the scenarios, but adding this switch is sort of common sense cleanup for both projects.

By default, umd unit tests are not built. If TT_UMD_BUILD_TESTS=ON then only build them, and if it is defined, they can check ARCH_NAME variable.

It won't be a drastic change from what already happens, based on their use of EXCLUDE_FROM_ALL token in their tests.

TT-billteng · 2024-09-27T23:06:46Z

you're changing a lot more than just controlling which tests to build in this PR, can you separate it out? We also currently have no way to test build_metal.sh so you need to be careful with the changes.

blozano-tt · 2024-09-27T23:51:46Z

you're changing a lot more than just controlling which tests to build in this PR, can you separate it out? We also currently have no way to test build_metal.sh so you need to be careful with the changes.

Guilty as charged.

Will separate it out.

tt-rkim · 2024-09-30T13:41:04Z

build_metal.sh

+
+build_dir="build_$build_type"
+
+
 # Set the python environment directory if not already set
 if [ -z "$PYTHON_ENV_DIR" ]; then


Is this needed in the CMake system? Don't think so right?

Not sure actually.

tt-rkim · 2024-09-30T13:41:52Z

CMakeLists.txt

@@ -273,7 +276,7 @@ install(TARGETS ttnn
 )
 if(WITH_PYTHON_BINDINGS)
    # Install .so into src files for pybinds implementation
-    install(FILES ${PROJECT_BINARY_DIR}/lib/_ttnn.so


How does the install target know which file to copy over? I don't see this declared anywhere else

The CMake target "ttnn" is a shared library target. CMake will always know where it is building it, so we don't have to tell CMake where it is. We just need to say "install target", just like we are doing a few lines above (line 272).

tt-rkim · 2024-09-30T13:42:39Z

.github/workflows/tg-unit-tests.yaml

@@ -37,7 +37,7 @@ jobs:
          echo "TT_METAL_HOME=$(pwd)" >> $GITHUB_ENV
      - name: Build UMD device and tests
        run: |
-          cmake -B build -G Ninja
+          cmake -B build -G Ninja -DTT_UMD_BUILD_TESTS=ON


What the heck - why are we building the tests on the TG machine? We should be downloading build artifacts. Sounds like an issue to write up to do after

Can you write up said issue? I don't know what a TG machine is yet.
Eventually, we don't want to be building UMD tests at all in tt-metal CI, because UMD Is an external third party library, and they are starting to run their own CI.

tt-rkim · 2024-09-30T13:43:18Z

build_metal.sh

-: '
-TLDR: Follow the steps outlined below to build metal. For more in-depth information, keep reading
-
-Steps:


There was really nothing from here you wanted to keep as comments?

Correct, I want a single source of truth on documentation.
Anything worth documenting should be in README.md.

tt-rkim · 2024-09-30T13:53:27Z

tt_metal/tools/watcher_dump/CMakeLists.txt

@@ -1,6 +1,6 @@

 add_executable(watcher_dump ${CMAKE_CURRENT_SOURCE_DIR}/watcher_dump.cpp)
-target_link_libraries(watcher_dump PRIVATE test_metal_common_libs)


Damn, good catch

Had to fix a build failure, after separating the libraries and executables from the tests.

blozano-tt · 2024-09-30T16:04:10Z

Splitting this PR into:
#13251 and #13259
As requested by @TT-billteng
Closing #13141

blozano-tt requested review from tt-rkim, vtangTT and TT-billteng as code owners September 26, 2024 05:54

blozano-tt commented Sep 26, 2024

View reviewed changes

tests/CMakeLists.txt Show resolved Hide resolved

blozano-tt temporarily deployed to dev September 26, 2024 06:04 — with GitHub Actions Inactive

blozano-tt had a problem deploying to dev September 26, 2024 06:04 — with GitHub Actions Failure

blozano-tt had a problem deploying to dev September 26, 2024 06:05 — with GitHub Actions Failure

ttmchiou self-requested a review September 26, 2024 17:13

blozano-tt requested review from abhullar-tt, pgkeller, aliuTT, tt-aho, tt-dma, tt-asaigal and ubcheema as code owners September 27, 2024 00:07

blozano-tt temporarily deployed to dev September 27, 2024 00:08 — with GitHub Actions Inactive

blozano-tt had a problem deploying to dev September 27, 2024 00:09 — with GitHub Actions Failure

blozano-tt had a problem deploying to dev September 27, 2024 00:09 — with GitHub Actions Error

blozano-tt had a problem deploying to dev September 27, 2024 00:09 — with GitHub Actions Failure

blozano-tt temporarily deployed to dev September 27, 2024 00:14 — with GitHub Actions Inactive

blozano-tt had a problem deploying to dev September 27, 2024 00:15 — with GitHub Actions Failure

blozano-tt had a problem deploying to dev September 27, 2024 00:15 — with GitHub Actions Error

blozano-tt had a problem deploying to dev September 27, 2024 00:15 — with GitHub Actions Failure

blozano-tt had a problem deploying to dev September 27, 2024 00:15 — with GitHub Actions Error

blozano-tt temporarily deployed to dev September 27, 2024 01:28 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 01:38 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 01:39 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 18:15 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 18:29 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 18:44 — with GitHub Actions Inactive

blozano-tt temporarily deployed to dev September 27, 2024 18:47 — with GitHub Actions Inactive

ttmchiou approved these changes Sep 27, 2024

View reviewed changes

blozano-tt added 6 commits September 27, 2024 15:40

#13128: Enhance build_metal.sh

f0ecf33

#13128: Update CMake infra to conditionally build tests

a5121e3

#13128: Fix build with tracy, update workflows

9b29728

#13128: Fix workflow

277ffac

Fix workflows

d826294

#13128: Optimize cmake switches, add more support to build script

449c93e

ttmchiou force-pushed the blozano/cmake_build_tests branch from b6b1ce8 to 449c93e Compare September 27, 2024 22:40

tt-rkim reviewed Sep 30, 2024

View reviewed changes

blozano-tt closed this Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#13128: Enhance build_metal.sh #13141

#13128: Enhance build_metal.sh #13141

blozano-tt commented Sep 26, 2024 •

edited

Loading

ttmchiou commented Sep 27, 2024

blozano-tt commented Sep 27, 2024

TT-billteng commented Sep 27, 2024

blozano-tt commented Sep 27, 2024

tt-rkim Sep 30, 2024

blozano-tt Sep 30, 2024

tt-rkim Sep 30, 2024

blozano-tt Sep 30, 2024

tt-rkim Sep 30, 2024

blozano-tt Sep 30, 2024

tt-rkim Sep 30, 2024

blozano-tt Sep 30, 2024

tt-rkim Sep 30, 2024

blozano-tt Sep 30, 2024

blozano-tt commented Sep 30, 2024

		@@ -1,6 +1,6 @@

		add_executable(watcher_dump ${CMAKE_CURRENT_SOURCE_DIR}/watcher_dump.cpp)
		target_link_libraries(watcher_dump PRIVATE test_metal_common_libs)

#13128: Enhance build_metal.sh #13141

#13128: Enhance build_metal.sh #13141

Conversation

blozano-tt commented Sep 26, 2024 • edited Loading

Ticket

Problem description

What's changed

Checklist

ttmchiou commented Sep 27, 2024

blozano-tt commented Sep 27, 2024

TT-billteng commented Sep 27, 2024

blozano-tt commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blozano-tt commented Sep 30, 2024

blozano-tt commented Sep 26, 2024 •

edited

Loading