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

Place the public API in a special place with a canonical way of accessing them #322

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

afuller-TT
Copy link
Collaborator

@afuller-TT afuller-TT commented Nov 21, 2024

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

  • Place all UMD Device's public API under api/umd/device/
  • Expose api/ as the include dir, so all consumers must use umd/device/foo.h to include API headers
    • Even internal to UMD::Device, though this is subjective.
  • Do not expose any other directory publicly
    • Inside Device we may include other headers, but external consumers cannot.

Testing

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:

Copy link
Contributor

@broskoTT broskoTT left a 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.

common/CMakeLists.txt Show resolved Hide resolved
common/utils.hpp Show resolved Hide resolved
device/CMakeLists.txt Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
device/blackhole/blackhole_implementation.cpp Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
@broskoTT broskoTT added the changes api API changing PR, needs changes in client code label Nov 22, 2024
@afuller-TT afuller-TT enabled auto-merge (squash) November 27, 2024 21:16
@afuller-TT afuller-TT merged commit aea0eea into main Nov 27, 2024
9 of 12 checks passed
@afuller-TT afuller-TT deleted the afuller/clarify-api branch November 27, 2024 21:19
@@ -1,7 +1,7 @@
// SPDX-FileCopyrightText: (c) 2023 Tenstorrent Inc.
//
// SPDX-License-Identifier: Apache-2.0
#include "cluster.h"
#include "umd/device/cluster.h"
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes api API changing PR, needs changes in client code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants