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

Creating ROCm whl upon release #259

Merged
merged 9 commits into from
Nov 1, 2024
Merged

Creating ROCm whl upon release #259

merged 9 commits into from
Nov 1, 2024

Conversation

gshtras
Copy link
Collaborator

@gshtras gshtras commented Nov 1, 2024

Creating ROCm versions of vllm and gradlib whl

Copy link

@Alexei-V-Ivanov-AMD Alexei-V-Ivanov-AMD left a comment

Choose a reason for hiding this comment

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

.github/workflows/scripts/build.sh:L4

Definitely "python_executable=python$1" is a cryptic way to interface the desirable python flavor (not ideal), but why we limit the script to python3 ?

Otherwise it looks good to me.

Copy link

@Alexei-V-Ivanov-AMD Alexei-V-Ivanov-AMD left a comment

Choose a reason for hiding this comment

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

Besides the comment it looks good to me.

@@ -1,23 +1,22 @@
#!/bin/bash
set -eux

python_executable=python$1

Choose a reason for hiding this comment

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

Definitely "python_executable=python$1" is a cryptic way to interface the desirable python flavor (not ideal), but why we limit the script to python3 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What other options are there in our ROCm release docker (or anywhere)?

Copy link

@Alexei-V-Ivanov-AMD Alexei-V-Ivanov-AMD Nov 1, 2024

Choose a reason for hiding this comment

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

python2 ? If somebody decides to use python2 at some point (for whatever reason) this code will break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code will not be within the top 1000 things that will break :)


# Build
$python_executable setup.py bdist_wheel --dist-dir=dist
cd gradlib
$python_executable setup.py bdist_wheel --dist-dir=dist
cd ..

Choose a reason for hiding this comment

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

This makes it perfect, doesn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll see if whls will get published this time.

Choose a reason for hiding this comment

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

That was the last time I approved without seeing functionality proof.

Copy link

@Alexei-V-Ivanov-AMD Alexei-V-Ivanov-AMD left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@gshtras gshtras merged commit 733f79a into main Nov 1, 2024
20 of 22 checks passed
@gshtras gshtras deleted the whl_build branch November 1, 2024 23:02
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.

2 participants