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

rocm6.3 fix for docker build and debug option for gpu code #157

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

maleksan85
Copy link

CMAKE_BUILD_TYPE=Debug python ./setup.py develop <- should produce code that in Rocm GDB can show all variables.

Plus fix for:
image

@maleksan85 maleksan85 force-pushed the rocm6.3_and_debug_improvements branch from 268fbcc to b0cc8cd Compare August 27, 2024 17:33
Dockerfile.rocm Outdated
@@ -25,8 +25,8 @@ ARG ARG_PYTORCH_ROCM_ARCH="gfx90a;gfx942"
ENV PYTORCH_ROCM_ARCH=${ARG_PYTORCH_ROCM_ARCH}

# Install some basic utilities
RUN apt-get update && apt-get install python3 python3-pip -
RUN apt-get update && apt-get install -y \
RUN apt-get update -q -y --force-yes && apt-get install -q -y --force-yes python3 python3-pip -
Copy link
Collaborator

Choose a reason for hiding this comment

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

The trailing "-" was supposed to be "-y" actually

Copy link
Author

Choose a reason for hiding this comment

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

hm... didn't know that :) will remove.

@@ -123,6 +123,11 @@ function (get_torch_gpu_compiler_flags OUT_GPU_FLAGS GPU_LANG)
"-U__HIP_NO_HALF_CONVERSIONS__"
"-U__HIP_NO_HALF_OPERATORS__"
"-fno-gpu-rdc")

string(TOUPPER ${CMAKE_BUILD_TYPE} VLLM_BUILD_TYPE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

set(CMAKE_HIP_FLAGS_DEBUG "${CMAKE_HIP_FLAGS_DEBUG} -O0 -ggdb3")
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -ggdb3")
probably cleaner

Copy link
Author

Choose a reason for hiding this comment

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

sorry, but I don't see where FLAGS_DEBUG are used now? Or you mean promote those flags into other places in CMakeLists.txt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dockerfile.rocm Outdated
RUN apt-get update && apt-get install python3 python3-pip -
RUN apt-get update && apt-get install -y \
RUN apt-get update -q -y --force-yes && apt-get install -q -y --force-yes python3 python3-pip -
RUN apt-get update -q -y --force-yes && apt-get install -q -y --force-yes \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want the force-yes? Could be better to catch the future errors that it would mask

Copy link
Author

@maleksan85 maleksan85 Aug 27, 2024

Choose a reason for hiding this comment

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

well, imo, when update asks for something it will break installation. Usually error is like (from one of the chats):
image
and to bypass it one needs those parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

force-yes means suppress incompatibility and other warning messages. To answer yes to the default apt-get prompt you just need the -y

Copy link
Collaborator

@gshtras gshtras left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about using VLLM_GPU_LANG. Other accelerator compilers may not be clang based. But then again why would they build ROCm/vllm for those, so meh

@maleksan85 maleksan85 merged commit c24a495 into main Aug 29, 2024
13 checks passed
@gshtras gshtras deleted the rocm6.3_and_debug_improvements branch September 10, 2024 19:24
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