-
Notifications
You must be signed in to change notification settings - Fork 107
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
Begin process of packaging PRAGmatic #597
Conversation
Reviewer's Guide by SourceryThis pull request adds the initial setup for packaging the PRAGmatic library. It clones the PRAGmatic repository, installs its dependencies, and installs the library itself. It also adds Sequence diagram for PRAGmatic build and installation processsequenceDiagram
participant S as Script
participant DNF as Package Manager
participant Git as Git
participant Pip as Pip
S->>DNF: Install Python
S->>Git: Clone PRAGmatic repository
S->>Git: Initialize submodules
S->>Pip: Install requirements
S->>Pip: Install PRAGmatic package
S->>S: Cleanup
Flow diagram for build script modificationsflowchart TD
A[Start] --> B[DNF Install]
B --> C[Install Python]
C --> D[Clone PRAGmatic]
D --> E[Update Submodules]
E --> F[Install Requirements]
F --> G[Install PRAGmatic]
G --> H[Build Whisper.cpp]
H --> I[Build Llama.cpp]
I --> J[Cleanup]
J --> K[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This could get painful: |
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.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Since this is marked as WIP, please clarify in the PR description what work is still pending and what the current limitations are.
- Consider using a virtual environment for pip installations instead of installing directly to /usr, or document why this specific approach is necessary.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -94,6 +95,18 @@ clone_and_build_llama_cpp() { | |||
cd .. | |||
} | |||
|
|||
clone_and_build_pragmatic() { | |||
local llama_cpp_sha="924518e2e5726e81f3aeb2518fb85963a500e93a" |
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.
Unused stuff could be cleaned up here
2fe8b8e
to
b04281b
Compare
@@ -0,0 +1,22 @@ | |||
#!/bin/bash | |||
|
|||
clone_and_build_pragmatic() { |
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.
We should add
set -exu -o pipefail
We want the build to fail if these commands fail.
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.
Ok, I am still working on making the install smaller. Gotten CPU cut in 1/3 but little change in ROCM and CUDA.
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.
Should we add the same flag to build_llama_and_whisper.sh?
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.
set -ex
is there, it's just in the main function (we could add, -o pipefail
, etc. if it suits). I tend to create main functions in shell scripts to encourage using scope and less global variable.
set -e
alone covers a lot to be fair sometimes it's sufficient.
I do like to add -x
at times also, makes CI easy to debug.
But set -e
at least is a good idea.
|
||
export PYTHON_VERSION="python3 -m" | ||
if [ "$(python3 --version)" \< "Python 3.11" ]; then | ||
dnf install -y python3.11 python3.11-pip |
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.
Question, python3.12 and python3.13 are not supported? Still learning these bits.
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.
They are supported, this is just checking for RHEL 9 systems with python 3.9. Basically ramalama requires 3.11 or later.
@ericcurtin @bmahabirbu this is ready to go in. It does not build anything yet, just allows people to build RAG Models layers on top of our images. Once this is in, we can start experimenting with using RAG with RamaLama. |
Building Pragmatic into a container image is fairly easy. podman build --build-arg IMAGE=quay.io/ramalama/rocm -t quay.io/ramalama/rocm-rag container-images/pragmatic Signed-off-by: Daniel J Walsh <[email protected]>
@bmahabirbu feel free to review and merge, I'll probably be offline soon (if the builds pass of course). Fedora 42 build can be ignored, would love to turn that off as it's unstable (Fedora 42 isn't even released). |
@rhatdan thanks for merging! Will play around with this this week! |
Here is the lastest stuff I am working on for creation of the vector.db |
I also just got this merged: redhat-et/PRAGmatic#39 |
Building Pragmatic into a container image is fairly easy.
podman build --build-arg IMAGE=quay.io/ramalama/rocm -t quay.io/ramalama/rocm-rag container-images/pragmatic
Summary by Sourcery
Build: