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

Simplify container image build #451

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Simplify container image build #451

merged 1 commit into from
Nov 15, 2024

Conversation

ericcurtin
Copy link
Collaborator

@ericcurtin ericcurtin commented Nov 12, 2024

Simplifying

Summary by Sourcery

Build:

  • Simplify the container image build script by consolidating package installation lists and removing redundant entries.

Simplifying

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
Copy link
Contributor

sourcery-ai bot commented Nov 12, 2024

Reviewer's Guide by Sourcery

This PR refactors the container image build script to improve organization and reduce redundancy in package installation logic. The changes primarily focus on consolidating package lists and simplifying the conditional installation blocks for different container types.

Sequence diagram for the refactored package installation process

sequenceDiagram
    participant User
    participant Script
    participant DNF

    User->>Script: Run build_llama_and_whisper.sh
    Script->>DNF: Install common RPM packages
    alt If containerfile is ramalama
        Script->>DNF: Install EPEL release
        Script->>DNF: Enable CRB
        Script->>DNF: Install additional packages for ramalama
    else If containerfile is asahi
        Script->>DNF: Enable Asahi branding
        Script->>DNF: Install additional packages for asahi
    else If containerfile is rocm
        Script->>DNF: Install ROCm packages
    else If containerfile is cuda
        Script->>DNF: Install CUDA packages
    end
Loading

File-Level Changes

Change Details Files
Reorganized package installation lists and consolidated common packages
  • Created a separate vulkan_rpms array for Vulkan-related packages
  • Moved common packages to the main rpm_list array
  • Removed duplicate package declarations across different container types
container-images/scripts/build_llama_and_whisper.sh
Simplified conditional installation logic for different container types
  • Improved URL variable reuse in the ramalama section
  • Streamlined package installation commands using array references
  • Consolidated Vulkan package installation across container types
container-images/scripts/build_llama_and_whisper.sh

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @ericcurtin - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@@ -1,33 +1,31 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Add error handling to ensure package installation failures don't go unnoticed

Consider adding 'set -e' at the start of the script and checking return values of dnf commands to fail fast if package installation fails.

Suggested change
#!/bin/bash
#!/bin/bash
set -e
dnf_install() {
local rpm_list=("python3" "python3-pip" "python3-argcomplete" \
"python3-dnf-plugin-versionlock" "gcc-c++" "cmake" "vim" \
"procps-ng" "git" "dnf-plugins-core")
local vulkan_rpms=("vulkan-headers" "vulkan-loader-devel" "vulkan-tools" \
"spirv-tools" "glslc" "glslang")

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of adding set -e to the script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your feedback, we will generate more comments like this in the future according to the following instructions:

- Ensure the comment provides a clear suggestion for improvement or enhancement.
- The comment should address potential issues such as error handling or code robustness.
- Include specific code examples or modifications to illustrate the suggestion.
- The comment should be relevant to the context of the code and provide actionable advice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already there, it's just the AI bot isn't smart enough to detect it:

main() {
  set -e

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:

- Ensure the comment provides a clear and actionable suggestion for improvement.
- Verify that the suggestion is relevant to the context of the code and addresses a potential issue or enhancement.
- Check if the comment includes specific code examples or modifications that illustrate the suggested change.
- Avoid comments that suggest changes without considering the broader implications or without sufficient justification.

Copy link
Member

Choose a reason for hiding this comment

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

It's Artificial Intelligence, but this does not mean it is smart.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:

- Ensure the comment provides a clear and actionable suggestion for improvement.
- Verify that the suggestion is relevant to the code and addresses a potential issue or enhancement.
- Check if the comment includes specific code examples or modifications that illustrate the suggestion.
- Avoid comments that suggest changes without considering the context or potential side effects.

@rhatdan
Copy link
Member

rhatdan commented Nov 15, 2024

LGTM

@rhatdan rhatdan merged commit 41d0c21 into main Nov 15, 2024
12 checks passed
@ericcurtin ericcurtin deleted the ramalama-container-fix branch November 15, 2024 10:55
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