Skip to content

Add MI300X Runners to AMD workflow. #149

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

Merged
merged 9 commits into from
Feb 16, 2025
Merged

Add MI300X Runners to AMD workflow. #149

merged 9 commits into from
Feb 16, 2025

Conversation

saienduri
Copy link
Collaborator

@saienduri saienduri commented Jan 22, 2025

Description

This commit adds mi300 nodes as an option to the current amd workflow.

I tested a couple times on the AMD workflow here (https://github.com/gpu-mode/discord-cluster-manager/actions/runs/12902691574)

Checklist

Before submitting this PR, ensure the following steps have been completed:

  • Run the slash command /verifyruns on your own server.
    • Run the cluster bot on your server:
      python discord-bot.py
    • Start training runs with the slash command /verifyruns.
    • Verify that the bot eventually responds with:
      ✅ All runs completed successfully!
      
      (It may take a few minutes for all runs to finish. In particular, the GitHub
      runs may take a little longer. The Modal run is typically quick.)
      For more information on running a cluster bot on your own server, see
      README.md.

@saienduri saienduri requested review from ngc92 and msaroufim January 22, 2025 06:50
@@ -13,10 +13,18 @@ on:

jobs:
run:
runs-on: [amdgpu-mi250-x86-64]
runs-on: ${{ matrix.runs-on }}
strategy:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the way we want to handle multiple runners; afaics, this well always run the given code sample on both types of AMD cards. Instead, the card should be selectable by an input argument that is given to the job.

Both runners should be registered in src/discord-cluster-manager/consts.py, and
https://github.com/gpu-mode/discord-cluster-manager/blob/main/src/discord-cluster-manager/cogs/github_cog.py#L35-L40
needs to be updated

Copy link
Collaborator Author

@saienduri saienduri Feb 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the info! Updated github_cog but I don't think we need to touch consts.py

@msaroufim
Copy link
Member

To merge this PR faster I'm thinking what might be easiest is just moving to only using MI300 - wdyt @saienduri

@saienduri
Copy link
Collaborator Author

Thanks for the signal @msaroufim! I updated the PR, so that it should work with card selection. Does it look good to y'all?
Here's an example run I trigged in github: https://github.com/gpu-mode/discord-cluster-manager/actions/runs/13350719364

@saienduri saienduri requested a review from ngc92 February 16, 2025 02:09
@msaroufim msaroufim merged commit 00a6331 into main Feb 16, 2025
4 checks passed
@saienduri saienduri deleted the mi300 branch February 16, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants