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

create-ethernet-map lib instead of bin #245

Merged
merged 11 commits into from
Nov 6, 2024
Merged

Conversation

broskoTT
Copy link
Contributor

@broskoTT broskoTT commented Nov 1, 2024

Related to #99

This change will simplify our build system changes by a lot. No additional artefacts needed to be shipped with libdevice, and no thinking about relative/absolute path once the lib is built.

@TTDRosen provided this library.

Changes:

  • Add lib to git, add it to build system.
  • Remove CEM binary from git
  • Add cluster yaml creation to tt_ClusterDescriptor
  • Use this interface in tests, remove deprecated GetClusterDescYAML

The following PR will remove cluster yaml altogether. Both from tt_ClusterDescriptor interface and from tt_SiliconDevice. Didn't want to overwhelm this PR.

This change requires client changes. Breaks existing references to create-ethernet-map binary

@broskoTT broskoTT added the changes api API changing PR, needs changes in client code label Nov 1, 2024
@joelsmithTT
Copy link
Contributor

Can you or @TTDRosen add instructions for generating this new binary and perhaps consider maintaining ARM64 support in UMD (which this change breaks), and possibly adding RISC-V support? There's community interest in the latter and I have hardware setups for both. (I did N150 home trials with an Ampere system and although I never merged the metal support it would be nice if this kept working)

@TTDRosen
Copy link
Contributor

TTDRosen commented Nov 1, 2024

I am not sure of the appropriate place to put this (someone please advise) but to reduce our bus factor.

From luwen root
cargo build -p create-ethernet-map --release --lib

@joelsmithTT
Copy link
Contributor

Great, thanks @TTDRosen. This change is fine with me provided that cargo command plus a link to Luwen is placed in a comment (maybe in the CMakeLists.txt?) so anyone with a non-X86 platform can build and swap out that binary.

device/CMakeLists.txt Outdated Show resolved Hide resolved
@blozano-tt blozano-tt force-pushed the brosko/create_ethernet_map_lib branch from dfae6f6 to 6d4e180 Compare November 2, 2024 08:25
@broskoTT broskoTT force-pushed the brosko/create_ethernet_map_lib branch from 6cfe55f to f86b0f1 Compare November 5, 2024 09:17
device/libs/README.md Outdated Show resolved Hide resolved
@broskoTT broskoTT merged commit 718132f into main Nov 6, 2024
19 checks passed
@broskoTT broskoTT deleted the brosko/create_ethernet_map_lib branch November 6, 2024 09:22
broskoTT added a commit that referenced this pull request Nov 13, 2024
### Issue
Fixes #298 

### Description
After #245 the default location of cluster_descriptor.yaml creation has
changed. There were some comments from metal and debuda team around
this. Mainly, this file is not hidden anymore.
On the suggestion from @tt-vjovanovic , I've changed the code to create
this file in OS provided temporary directory.

### List of the changes
- Put newly created cluster_descriptor.yaml in a temp folder. This won't
be relative to working dir anymore.

### Testing
CI tests touch this codepath

### API Changes
There are no API changes in this PR.
broskoTT added a commit that referenced this pull request Nov 13, 2024
### Issue
Fixes #298 

### Description
After #245 the default location of cluster_descriptor.yaml creation has
changed. There were some comments from metal and debuda team around
this. Mainly, this file is not hidden anymore.
On the suggestion from @tt-vjovanovic , I've changed the code to create
this file in OS provided temporary directory.

### List of the changes
- Put newly created cluster_descriptor.yaml in a temp folder. This won't
be relative to working dir anymore.

### Testing
CI tests touch this codepath

### API Changes
There are no API changes in this PR.
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.

6 participants