-
Notifications
You must be signed in to change notification settings - Fork 7
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
Place the public API in a special place with a canonical way of accessing them #322
Conversation
3e3ca49
to
c8ae6be
Compare
07d2c68
to
6bdc44f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome effort, thanks! And thanks for honouring the tt-metal and tt-debuda required changes!
I've left some comments, but they're for my clarification, none of them call to changes in this PR.
@@ -1,7 +1,7 @@ | |||
// SPDX-FileCopyrightText: (c) 2023 Tenstorrent Inc. | |||
// | |||
// SPDX-License-Identifier: Apache-2.0 | |||
#include "cluster.h" | |||
#include "umd/device/cluster.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is umd/device
not part of the build include path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we haven't discussed what internal access should look like. This PR was demonstrating external access and putting all the public headers in a segregated place to distinguish external vs internal. That aspect is more straightforward / objective.
Internal access is more subjective.
On the one hand, accessing the same way as an external consumer makes it obvious when we're using a public API -- does that matter? For some projects maybe that's useful information and for others it's inconsequential.
It also means that moving a file from internal to external doesn't require changing the includes. In a project highly in flux, perhaps that's a useful convenience. But if structing include ordering to indicate proximity, then it'll need to be updated anyway. And in a more well established project hoisting a module externally is very very rare in the grand scheme of things.
On the other hand, accessing like any other internal header separates on a different concern: I'm conveying that this is a header within this project, but I'm not affected nor do I care whether said header is ALSO available externally or not. Not my concern.
I believe @blozano-tt prefers the former. I prefer the latter. But ultimately neither of us own UMD, so it's you and @broskoTT who should make a call re: what you prefer internal for your project. I can help guide on the CMake change for whichever you prefer.
Issue
Solving part of #324
Description
It's currently unclear which files consist of UMD's public API and which are private headers.
Additionally, consumers like tt-metal are reaching around with arbitrary paths to fetch headers. They shouldn't.
There should be one place for the public API and one way for consumers to access it, and private stuff should remain private.
List of the changes
api/umd/device/
api/
as the include dir, so all consumers must useumd/device/foo.h
to include API headersTesting
Corresponding PR in tt-metal to consume with the new paths:
API Changes
(When making API changes, don't merge this PR until tt_metal and tt_debuda PRs are approved.)
(Then merge this PR, change the client PRs to point to UMD main, and then merge them.)
(Remove following lines if untrue) This PR has API changes: