Fix up building and pushing OCI Images #492

rhatdan · 2024-11-25T21:52:20Z

Summary by Sourcery

Refactor the OCI image building and pushing process to separate the build and manifest creation steps, and fix the push command to handle image names correctly.

Bug Fixes:

Fix the push command to correctly handle source and target image names, ensuring proper image conversion and manifest creation.

Enhancements:

Refactor the OCI image building process to separate the build and manifest creation steps, improving code clarity and maintainability.

sourcery-ai · 2024-11-25T21:52:25Z

Reviewer's Guide by Sourcery

This PR refactors the OCI image building and pushing functionality to improve error handling and provide better separation of concerns. The changes split the build process into distinct steps (build, create manifest, and convert) while also improving the push operation's error messages and handling.

Sequence diagram for OCI image build and push process

sequenceDiagram
    participant User
    participant CLI
    participant OCI
    participant Conman
    User->>CLI: push_cli(args)
    CLI->>OCI: New(tgt, args)
    OCI->>OCI: push(source, args)
    alt Source is not target
        OCI->>OCI: _convert(source, target, args)
        OCI->>OCI: _build(source, target, args)
        OCI->>Conman: build image
        Conman-->>OCI: imageid
        OCI->>OCI: _create_manifest(target, imageid, args)
        OCI->>Conman: create manifest
        OCI->>Conman: add image to manifest
    end
    OCI->>Conman: push image
    Conman-->>OCI: success or error
    OCI-->>CLI: success or error
    CLI-->>User: success or error

Updated class diagram for OCI class

classDiagram
    class OCI {
        -conman
        -model
        +push(source, args)
        +pull(args)
        -_build(source, target, args) : string
        -_create_manifest(target, imageid, args)
        -_convert(source, target, args)
    }
    note for OCI "Refactored to separate build, create manifest, and convert steps"

File-Level Changes

Change	Details	Files
Refactored OCI image building process into separate methods	Added _create_manifest method to handle manifest operations Added _convert method to handle image conversion process Modified _build method to return image ID Added --no-cache flag to build command for consistency	`ramalama/oci.py`
Improved push operation implementation	Simplified push command argument construction Enhanced error messages to be more specific Streamlined push logic flow Fixed target/source handling in push operation	`ramalama/oci.py`
Updated CLI push handling	Changed error catching from KeyError to NotImplementedError Fixed model type checking logic to use correct variable Corrected parameter passing in OCI push fallback	`ramalama/cli.py`

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

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @rhatdan - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider using proper logging instead of print statements for better operability and consistency with library practices
The error handling in _convert() could be more specific - catching all CalledProcessError instances might mask important issues

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟢 Testing: all looks good
🟡 Complexity: 1 issue found
🟢 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.}

sourcery-ai · 2024-11-25T21:53:18Z

ramalama/oci.py

+        except subprocess.CalledProcessError as e:
+            pass
+        imageid = self._build(source, target, args)
+        imageid = self._create_manifest(target, imageid, args)


issue (bug_risk): The _create_manifest method doesn't return a value, making this assignment incorrect

ramalama/oci.py

rhatdan · 2024-11-26T15:07:57Z

@nalind PTAL

saschagrunert · 2024-11-26T15:17:20Z

ramalama/oci.py

+        cmd_args = [
+            self.conman,
+            "manifest",
+            "create",
+            "--annotation",
+            f"{annotations.AnnotationModel}=true",
+            "--annotation",
+            f"{ocilabeltype}=''",
+            "--annotation",
+            f"{annotations.AnnotationTitle}=args.SOURCE",
+            target,
+        ]
+        run_cmd(cmd_args, stdout=None, debug=args.debug)
+        run_cmd([self.conman, "manifest", "add", target, imageid], stdout=None, debug=args.debug)


Can/should we set the artifact-type here?

Probably, one issue I am having is Docker support for this stuff kind of sucks.

I think I will just list all manifest lists as AI Models via ramalama list when using Docker, and then I can handle annotations differently with Podman.

rhatdan · 2024-11-26T18:44:45Z

With the new functionality we have to force the use of podman 5.0 for creation of OCI Images. Anything less is not working properly. This means we need Ubuntu 24.10 or newer, which kind of stinks.

rhatdan · 2024-11-28T11:10:17Z

@ericcurtin @saschagrunert @lsm5 I am thinking the tests are never running because github can not find VMs with the newer version of Ubuntu on it. The issue I have is that the features added in this PR require a newer version of Podman to run "Podman 5." and ubuntu 24.04 only has Podman 4.9

I can make RamaLama work with the Docker path for Podman < 5.0 but we will not end up with annotations in the OCI images.

lsm5 · 2024-11-28T11:17:42Z

@ericcurtin @saschagrunert @lsm5 I am thinking the tests are never running because github can not find VMs with the newer version of Ubuntu on it. The issue I have is that the features added in this PR require a newer version of Podman to run "Podman 5." and ubuntu 24.04 only has Podman 4.9

Yup, that's where testing-farm / TMT work much better. You get the latest fedora and centos OOTB and can be triggered via packit. I see there's a testing-farm job for centos-stream already.

RE: pushing images, IIUC, tmt folks are working on secrets handling feature, so if pushing can be handled separately, TMT should be able to handle everything else.

lsm5 · 2024-11-28T11:27:34Z

Yup, that's where testing-farm / TMT work much better. You get the latest fedora and centos OOTB and can be triggered via packit. I see there's a testing-farm job for centos-stream already.

NVM, I see there are other testing-farm jobs already. Let me check with testing-farm people about latest on secrets support.

lsm5 · 2024-11-28T11:45:42Z

NVM, I see there are other testing-farm jobs already. Let me check with testing-farm people about latest on secrets support.

secrets support seems to be implemented in testing-farm but not yet deployed. https://issues.redhat.com/browse/TFT-2472 . So, I guess hopefully soon.

nalind · 2024-12-02T19:30:56Z

docs/ramalama.1.md

@@ -19,7 +19,7 @@ Running in containers eliminates the need for users to configure the host system

 RamaLama pulls AI Models from model registries. Starting a chatbot or a rest API service from a simple single command. Models are treated similarly to how Podman and Docker treat container images.

-When both Podman and Docker are installed, RamaLama defaults to Podman, The `RAMALAMA_CONTAINER_ENGINE=docker` environment variable can override this behavior. When neither are installed RamaLama attempts to run the model with software on the local system.
+When both Podman and Docker are installed, RamaLama defaults to Podman, The `RAMALAMA_CONTAINER_ENGINE=docker` environment variable can override this behaviour. When neither are installed RamaLama attempts to run the model with software on the local system.


Both are correct depending on where you are.

Yeah I've noticed this a few times, UK Vs US English. IMO just merge, because both are correct !

nalind · 2024-12-02T19:31:05Z

ramalama/oci.py

+        return imageid
+
+    def tag(self, imageid, target, args):
+        # Create manifest list for target with imageid


Copy/pasted comment?

This allows us to assign annotations to help identify the model. Pull in annotations for the OCI model specification defined in https://github.com/CloudNativeAI/model-spec Then make all of the commands handle models stored in manifest list as well as images. Fix up building and pushing OCI Images Signed-off-by: Daniel J Walsh <[email protected]>

sourcery-ai bot reviewed Nov 25, 2024

View reviewed changes

rhatdan force-pushed the run branch 3 times, most recently from 2a83126 to 3921547 Compare November 26, 2024 15:05

saschagrunert approved these changes Nov 26, 2024

View reviewed changes

saschagrunert reviewed Nov 26, 2024

View reviewed changes

rhatdan force-pushed the run branch 4 times, most recently from 2cd7616 to a2eadee Compare November 26, 2024 17:04

rhatdan force-pushed the run branch 2 times, most recently from b77e10f to 34af17e Compare November 27, 2024 15:41

rhatdan force-pushed the run branch 7 times, most recently from be65b97 to 461cb2e Compare December 2, 2024 19:21

nalind reviewed Dec 2, 2024

View reviewed changes

rhatdan force-pushed the run branch from 461cb2e to 0811a7c Compare December 3, 2024 14:18

rhatdan force-pushed the run branch from 0811a7c to 11d4df9 Compare December 3, 2024 14:34

rhatdan merged commit c3a6c04 into containers:main Dec 3, 2024
10 of 11 checks passed

gaius-qi mentioned this pull request Dec 6, 2024

Define model spec together #504

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix up building and pushing OCI Images #492

Fix up building and pushing OCI Images #492

rhatdan commented Nov 25, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Nov 25, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

sourcery-ai bot Nov 25, 2024

rhatdan commented Nov 26, 2024

saschagrunert Nov 26, 2024

rhatdan Nov 26, 2024

rhatdan Nov 26, 2024

rhatdan commented Nov 26, 2024

rhatdan commented Nov 28, 2024

lsm5 commented Nov 28, 2024

lsm5 commented Nov 28, 2024

lsm5 commented Nov 28, 2024

nalind Dec 2, 2024

ericcurtin Dec 3, 2024

nalind Dec 2, 2024

Fix up building and pushing OCI Images #492

Fix up building and pushing OCI Images #492

Conversation

rhatdan commented Nov 25, 2024 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Nov 25, 2024 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for OCI image build and push process

Updated class diagram for OCI class

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Nov 25, 2024

Choose a reason for hiding this comment

rhatdan commented Nov 26, 2024

saschagrunert Nov 26, 2024

Choose a reason for hiding this comment

rhatdan Nov 26, 2024

Choose a reason for hiding this comment

rhatdan Nov 26, 2024

Choose a reason for hiding this comment

rhatdan commented Nov 26, 2024

rhatdan commented Nov 28, 2024

lsm5 commented Nov 28, 2024

lsm5 commented Nov 28, 2024

lsm5 commented Nov 28, 2024

nalind Dec 2, 2024

Choose a reason for hiding this comment

ericcurtin Dec 3, 2024

Choose a reason for hiding this comment

nalind Dec 2, 2024

Choose a reason for hiding this comment

rhatdan commented Nov 25, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Nov 25, 2024 •

edited

Loading