Moved pruning protocol from model to factory

[engelmi]

By moving the pruning of the protocol from the model input to the model_factory and encapsulating it in a dedicated function, unit tests can be written more easily.

Summary by Sourcery

Moves the protocol parsing logic from the model classes to the model factory to improve testability and code organization. It introduces a prune_model_input method in the ModelFactory class to remove the protocol from the model input string. It also adds a validate_oci_model_input method to validate OCI model inputs. Additionally, it includes new unit tests to validate the new logic.

Enhancements:

• The protocol parsing logic has been moved from the model classes to the model factory, and is now encapsulated in a dedicated function.

Tests:

• Added unit tests for the OCI model input validation.
• Added unit tests for the protocol pruning logic.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request refactors the model input parsing logic by moving the protocol pruning functionality from the model classes to the ModelFactory. It also introduces unit tests for the new functionality and a utility function for removing substrings.

Sequence diagram for Model Creation with Protocol Pruning

sequenceDiagram
    participant Client
    participant ModelFactory
    participant Huggingface
    participant Ollama
    participant OCI
    participant URL

    Client->>ModelFactory: create(model, transport, engine)
    ModelFactory->>ModelFactory: prune_model_input(model, cls)
    alt model.startswith("huggingface://") or model.startswith("hf://") or model.startswith("hf.co/")
        ModelFactory->>Huggingface: Huggingface(pruned_model)
        ModelFactory-->>Client: Huggingface
    else model.startswith("ollama://") or "ollama.com/library/" in model
        ModelFactory->>Ollama: Ollama(pruned_model)
        ModelFactory-->>Client: Ollama
    else model.startswith("oci://") or model.startswith("docker://")
        ModelFactory->>ModelFactory: validate_oci_model_input()
        ModelFactory->>OCI: OCI(pruned_model, engine)
        ModelFactory-->>Client: OCI
    else model.startswith("http://") or model.startswith("https://") or model.startswith("file://")
        ModelFactory->>URL: URL(pruned_model)
        ModelFactory-->>Client: URL
    else transport == "huggingface"
        ModelFactory->>Huggingface: Huggingface(pruned_model)
        ModelFactory-->>Client: Huggingface
    else transport == "ollama"
        ModelFactory->>Ollama: Ollama(pruned_model)
        ModelFactory-->>Client: Ollama
    else transport == "oci"
        ModelFactory->>ModelFactory: validate_oci_model_input()
        ModelFactory->>OCI: OCI(pruned_model, engine)
        ModelFactory-->>Client: OCI
    end

Loading

Updated class diagram for ModelFactory

classDiagram
    class ModelFactory {
        - model: str
        - transport: str
        - engine: str
        + __init__(model: str, transport: str, engine: str)
        + create(): Union[Huggingface, Ollama, OCI, URL]
        + prune_model_input(cls: type[Union[Huggingface, Ollama, OCI, URL]]) : str
        + validate_oci_model_input()
    }
    class Huggingface {

    }
    class Ollama {

    }
    class OCI {

    }
    class URL {

    }
    ModelFactory --|> Huggingface : creates
    ModelFactory --|> Ollama : creates
    ModelFactory --|> OCI : creates
    ModelFactory --|> URL : creates

    note for ModelFactory "The ModelFactory class now includes a prune_model_input method to remove the protocol from the model input string."

Loading

File-Level Changes

Change	Details	Files
Moved the protocol pruning logic from the model classes to the ModelFactory class.	Created a prune_model_input method in ModelFactory to remove the protocol from the model input string. Modified the create method in ModelFactory to use the prune_model_input method before instantiating model classes. Removed the protocol pruning logic from the __init__ methods of Huggingface, Ollama, and URL classes. Added a validate_oci_model_input method to validate OCI model inputs. Modified the OCI class to remove the protocol validation logic.	ramalama/model_factory.py ramalama/huggingface.py ramalama/ollama.py ramalama/url.py ramalama/oci.py
Introduced a utility function for removing substrings from a string.	Created a rm_until_substring function in ramalama/common.py to remove a substring from the beginning of a string. Removed the rm_until_substring function from ramalama/model.py.	ramalama/common.py ramalama/model.py
Added unit tests for the ModelFactory class and the rm_until_substring function.	Added new test cases to test_model_factory.py to test the prune_model_input method. Added new test cases to test_model_factory.py to test the validate_oci_model_input method. Created a new test file test_common.py with test cases for the rm_until_substring function.	test/unit/test_model_factory.py test/unit/test_common.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. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the 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 exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!
• Generate a plan of action for an issue: Comment @sourcery-ai plan on
  an issue to generate a plan of action for it.

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 @engelmi - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider adding a helper function to construct the Input dataclass in your tests to reduce duplication.
• The validate_oci_model_input method could be a standalone function instead of a method on the ModelFactory class.

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 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 ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[engelmi]

The ramalama convert file to image test fails, but I am not sure this is related to this change:

[07:52:37.127614592] $ /home/runner/work/ramalama/ramalama/bin/ramalama convert /tmp/ramalama_bats.yyeHxU/aimodel foobar
# [07:52:37.962614926] Error: invalid repository name (1b572995d33b4edde32e4b0112342f32bbd43dcbe5df317d452d962e3b15c691), cannot specify 64-byte hexadecimal strings
# Failed to create manifest for OCI foobar : Command '['podman', 'manifest', 'create', 'foobar', '1b572995d33b4edde32e4b0112342f32bbd43dcbe5df317d452d962e3b15c691']' returned non-zero exit status 125.
# Converting /tmp/ramalama_bats.yyeHxU/aimodel to foobar...
# Building foobar...

@ericcurtin WDYT?

Update:
Failure is related to the change. I missed that OCI-Models are instantiated without the use of New(). Fixing that.

[ericcurtin]

It's probably not for this PR, but it would be better if we resolved all environment variables, configuration files, CLI args, once early in execution of ramalama. I started that here at one point, but it drifted:

load_and_merge_config

we tend to do things like, use the "engine" value or container_manager() over and over again in this codebase which isn't great.

[engelmi]

I agree and good to know. I'll keep that in mind and will try to refactor occurrences like this.

[engelmi]

config here is already the result of load_and_merge_config where we do exactly the same thing. Maybe a dataclass or something similar might help. Although its less flexible, the fields are stated explicitly and don't encourage specifying defaults when the field isn't there.

[ericcurtin]

Yeah we should probably be doing something like:

config['engine']

here. The code is misleading, it suggests we might not have resolved this value correctly up to this point.