tenstorrent / tt-umd

User-Mode Driver for Tenstorrent hardware
Apache License 2.0
9 stars 6 forks source link

Consider working on a separate repo containing common utils #78

Open broskoTT opened 1 month ago

broskoTT commented 1 month ago

To be consumed by both tt-metal and tt-umd. There is a significant amount of code which is used at multiple places (previously Buda also), and there is an opportunity to have these in a separate repo, so that code is not copied around.

Copying original @vtangTT's comment: https://github.com/tenstorrent/tt-umd/issues/55#issuecomment-2363908875

I feel like we should use the same error handling as metal does rn with TT_FATAL, TT_ASSERT, etc.

There might be an opportunity to consolidate these utility functions as metal and umd both define their own rn in common/assert.hpp or common/logger.hpp and it gets confusing to tell which one people are including.

Maybe we can consider a third party utils repo that gets included in both metal and umd as a submodule so we don't have duplicate code?

@pjanevskiTT just stumbled upon another place where this makes sense in #75 , related to "driver_atomics.h", which is used/included in tt-metal, but this code is not tied to communication with our TT hardware, and should probably be moved outside.

This would involve creating another repo to hold such code, and then consuming it in metal and umd.

The common code could be lightweight enough to be a header only library.

broskoTT commented 1 month ago

@joelsmithTT @abhullar-tt , any thoughts?

abhullar-tt commented 1 month ago

I like this idea!

broskoTT commented 4 weeks ago

@blozano-tt Since you're fresh with the new ideas from tt-metal perspective, what do you think about this? You recently worked on deduplicating fmt, which seems related...

blozano-tt commented 4 weeks ago

@broskoTT it sounds like a good idea. Provides uniform logging across both repos, and avoids duplication.

blozano-tt commented 4 weeks ago

Sadly, there is quite a bit of divergence between tt-metal/tt_metal/common/logger.hpp and umd/common/logger.hpp

joelsmithTT commented 4 weeks ago

A nearer-term priority should be improving UMD logging rather than sharing components between UMD and Metal. While such sharing seems appealing, it introduces complexity and integration challenges. Let's first address UMD's logging problems directly.

I've encountered situations in UMD development where better logging would be a huge value add. My suggestion is for us to integrate a decent logging library (there are others; those are the ones I vouch for) into UMD rather than fixing up the existing logger.hpp or trying to use Metal's (which is even worse: it lacks timestamps, source location, and does not appear to be thread safe).

After we are benefitting from integrating a good logger (and have lived with it for a while, figured out how we like it, etc.), we can look into hoisting it out into a shared repo.

blozano-tt commented 4 weeks ago

A nearer-term priority should be improving UMD logging rather than sharing components between UMD and Metal. While such sharing seems appealing, it introduces complexity and integration challenges. Let's first address UMD's logging problems directly.

I've encountered situations in UMD development where better logging would be a huge value add. My suggestion is for us to integrate a decent logging library (there are others; those are the ones I vouch for) into UMD rather than fixing up the existing logger.hpp or trying to use Metal's (which is even worse: it lacks timestamps, source location, and does not appear to be thread safe).

After we are benefitting from integrating a good logger (and have lived with it for a while, figured out how we like it, etc.), we can look into hoisting it out into a shared repo.

We could create repo tenstorrent/tt-log

Create a basic wrapper around spdlog.

Integrate it into UMD, and then tt-metal.

A lot would be cleaned up along the way.

blozano-tt commented 4 weeks ago

@joelsmithTT @broskoTT

Thinking about it more…

It might be easier and more time efficient just to directly integrate spdlog into UMD.

Earlier, I was thinking we should abstract away our choice of logging library, and hide it in an interface library like tt-logger.

However, that creates work, and may limit the raw feature set of the open source logging library.

spdlog uses fmt under the hood, and supports variadic args like printf.

Should we just plan for that?

broskoTT commented 4 weeks ago

I think there are other parts of the code which are "utility" which would benefit from being in a separate repository. Of course, the balance is between the cost of managing new repository and how much stuff is actually there.

That said, regarding logging, I agree that if we use some other logger it seems like a burden to create another repo just to wrap it around, I'd rather we integrate it directly.

pjanevskiTT commented 6 days ago

Has things in common with #244

broskoTT commented 6 days ago

Related conversation: https://tenstorrent.slack.com/archives/C07L16Z59AP/p1730310644890619