tenstorrent / tt-metal

:metal: TT-NN operator library, and TT-Metalium low level kernel programming model.
https://docs.tenstorrent.com/ttnn/latest/index.html
Apache License 2.0
485 stars 79 forks source link

Remove built cache of previous git commits. #15344

Open spoojaryTT opened 4 days ago

spoojaryTT commented 4 days ago

Ticket

Link to Github Issue

Problem description

Previously built files might still be reused when the tt_metal is updated.

What's changed

  1. During build, in cmake, check git hash and add it to defines(GIT_COMMIT_HASH) for build.cpp (if git command returns error, no #define is set).
  2. Build.cpp would check for the git hash define (GIT_COMMIT_HASH), if hash is not found, proceed without changing any previously built folders and root folder will be 'built/'
  3. If git hash is found, the root folder will be 'built/GIT_COMMIT_HASH'.
  4. If git hash is found, and TT_METAL_SKIP_DELETING_BUILT_CACHE is NOT set, delete all directories in 'built' except for 'built/GIT_COMMIT_HASH' folder
  5. If git hash is found, and TT_METAL_SKIP_DELETING_BUILT_CACHE is set, no directory is deleted. Root for current build will be 'built/GIT_COMMIT_HASH'

Checklist

blozano-tt commented 3 days ago

I'm good with this. The CMake changes look good (once we fix the name of git).

I'm not familiar with the JIT build and haven't combed through the code to know why we're loading stale items. This approach feels like a hammer to me, but again I don't know the nuances so you know what's needed here.

I also find it weird.

I asked previously why we can't just put things in /tmp/ and let them get deleted after the process completes.

tt-rkim commented 3 days ago

We can't really test this change in CI without doing another build since it's a compile define

spoojaryTT commented 3 days ago

We can't really test this change in CI without doing another build since it's a compile define

If we want to avoid compile define, we can move finding of git hash to inside metal code. Use system() and pipe the output to a file. With this, we can probably test that root path is as expected. Thoughts @tt-rkim @pgkeller

tt-rkim commented 6 hours ago

Please do not add runtime code to get source control information. That'll break pipelines, anyway.

I'm more concerned of how you're gonna write an automated test for this at all.

Also, once @afuller-TT 's changes from here https://github.com/tenstorrent/tt-metal/pull/15400/files go in, we can change your CMake changes to parse data from VERSION_HASH so it's all in one place.

Can we address @blozano-tt and @afuller-TT 's comments about this being a big hammer? I know we've thought about this before. Are there no smaller hammers we can use to invalidate the cache of previous commits?

pgkeller commented 3 hours ago

Please do not add runtime code to get source control information. That'll break pipelines, anyway.

I'm more concerned of how you're gonna write an automated test for this at all.

Also, once @afuller-TT 's changes from here https://github.com/tenstorrent/tt-metal/pull/15400/files go in, we can change your CMake changes to parse data from VERSION_HASH so it's all in one place.

Can we address @blozano-tt and @afuller-TT 's comments about this being a big hammer? I know we've thought about this before. Are there no smaller hammers we can use to invalidate the cache of previous commits?

discussion prior to the PR (on devlopers channel, I think) people were worried about leaving stale files behind, hence the delete. the way I see it: 1) We need a key in the path. how we get the key I don't care, sounds like we need to wait for @afuller-TT's change 2) Optionally we can clean up. Sounds like developers want that and you don't. We can just delete that

does this work?