-
Notifications
You must be signed in to change notification settings - Fork 195
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
Add docker_compose example for AMD ROCm deployment #1053
Add docker_compose example for AMD ROCm deployment #1053
Conversation
0254696
to
80820e4
Compare
Please do the same test like this #1054 |
5ca7097
to
b40c970
Compare
Something wrong with this PR? The changed files are not related to ROCm. |
Probably changes was pulled from "main" because of git re-base, Let's close this one and #1054, and I'll prepare new |
OK, please paste the new PR link and close the old one. |
I suggest to use separate PRs for different examples, it's more efficiency for CI test. |
Signed-off-by: astafevav <[email protected]>
b40c970
to
ec1f0ab
Compare
Looks like i've succeed to reset and re-commit my branch in forked repo. Also resolved comments regarding test and test file name mentioned in DocSum PR(#1054) @chensuyue can we continue working on this PR ? |
Yes, we can continue use this PR. Let's confirm the connection of the test machine and run the test. |
cap_add: | ||
- SYS_PTRACE | ||
group_add: | ||
- video | ||
security_opt: | ||
- seccomp:unconfined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended for development, not production, as it disables security measures instead of adding them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AMD KB recommend to use this option for HPC environments to enable enables memory mapping,
Probably will change in the future when GPU path-through method will be changed to use "--gpus" option for docker and docker compose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the PTRACE? Surely that's needed only for debugging, and better done with separate container spec adding just that capability, and other extra tooling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TGI app is crashing when trying to run rocm image without PTRACE, so this capability is still needed,
All docker options were taken from PyTorch installation for ROCm manual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TGI app is crashing when trying to run rocm image without PTRACE, so this capability is still needed,
Ok.
That's very odd though. Any idea why it requires that, or do you have a backtrace where it's crashing?
|
||
stop_docker | ||
|
||
if [[ "$IMAGE_REPO" == "opea" ]]; then build_docker_images; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to make all scripts shellcheck
clean (apt install shellscheck; shellcheck *.sh
).
Co-authored-by: Eero Tamminen <[email protected]> Signed-off-by: astafevav <[email protected]>
Co-authored-by: Eero Tamminen <[email protected]> Signed-off-by: astafevav <[email protected]>
for more information, see https://pre-commit.ci
11efeda
to
03564bc
Compare
Please fix the CI issue. |
Please check the CI issue. https://github.com/opea-project/GenAIExamples/actions/runs/11795757034/job/32856177986?pr=1053 |
Please make sure each commit has been signed with "git commit -s", or the DCO check will failed. |
Will this PR also replaced by a new one? like #1054 |
yes, I will create new separate PR for FaqGen and put link to new PR here. |
Re-submitted PR: #1126 |
Type of change