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

Consider working on a separate repo containing common utils #78

Open
broskoTT opened this issue Sep 25, 2024 · 11 comments
Open

Consider working on a separate repo containing common utils #78

broskoTT opened this issue Sep 25, 2024 · 11 comments
Assignees
Labels
P2 Minor issues

Comments

@broskoTT
Copy link
Contributor

broskoTT commented Sep 25, 2024

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:
#55 (comment)

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
Copy link
Contributor Author

@joelsmithTT @abhullar-tt , any thoughts?

@abhullar-tt
Copy link
Contributor

I like this idea!

@broskoTT
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@blozano-tt
Copy link
Collaborator

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

@joelsmithTT
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link
Contributor

Has things in common with #244

@pjanevskiTT pjanevskiTT added the P2 Minor issues label Nov 5, 2024
@broskoTT
Copy link
Contributor Author

broskoTT commented Nov 6, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Minor issues
Projects
None yet
Development

No branches or pull requests

5 participants