-
Notifications
You must be signed in to change notification settings - Fork 124
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
Added model factory #874
Added model factory #874
Conversation
Reviewer's Guide by SourceryThis PR introduces a Sequence diagram for Model creation using ModelFactorysequenceDiagram
participant CLI
participant ModelFactory
participant Huggingface
participant Ollama
participant OCI
participant URL
CLI->>ModelFactory: create(model, transport, engine)
alt model is Huggingface
ModelFactory->>Huggingface: Huggingface(model)
ModelFactory-->>CLI: Huggingface
else model is Ollama
ModelFactory->>Ollama: Ollama(model)
ModelFactory-->>CLI: Ollama
else model is OCI
ModelFactory->>OCI: OCI(model, engine)
ModelFactory-->>CLI: OCI
else model is URL
ModelFactory->>URL: URL(model)
ModelFactory-->>CLI: URL
end
Updated class diagram for Model and ModelBaseclassDiagram
class ModelBase {
<<abstract>>
+login(args)
+logout(args)
+pull(args)
+push(source, args)
+remove(args)
+bench(args)
+run(args)
+perplexity(args)
+serve(args)
+exists(args)
+inspect(args)
}
class Model {
-model: str
-directory: str
-filename: str
+__init__(model)
+is_symlink_to(file_path, target_path)
+attempt_to_use_versioned(conman, image, vers, args)
+_image(args)
}
Model --|> ModelBase : extends
Class diagram for ModelFactoryclassDiagram
class ModelFactory {
-model: str
-transport: str
-engine: str
+__init__(model: str, transport: str, engine: str)
+create()
}
ModelFactory --|> Huggingface : creates
ModelFactory --|> Ollama : creates
ModelFactory --|> OCI : creates
ModelFactory --|> URL : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @engelmi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider renaming
integration-tests
toend-to-end-tests
to better reflect what they test. - The
__not_implemented_error
method inModelBase
could be a static method or defined outside the class since it doesn't useself
.
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
72d983a
to
268fbf3
Compare
Not sure why the help test for a bogus transport fails, expected and actual look the same to me: # #| FAIL: Verify bogus transport throws error
# #| expected: 'Error: transport "e_t12-6btz32rk" not supported. Must be oci, huggingface, or ollama.'
# #| actual: 'Error: Transport "e_t12-6btz32rk" not supported. Must be oci, huggingface, or ollama.' Edit: Never mind, found it... kind of playing "finding waldo". |
Signed-off-by: Michael Engel <mengel@redhat.com>
Signed-off-by: Michael Engel <mengel@redhat.com>
Signed-off-by: Michael Engel <mengel@redhat.com>
268fbf3
to
996e6f5
Compare
LGTM |
This PR includes the following changes:
The factory encapsulates the model creation and enables writing unit tests more easily. A few unit tests have been added to verify the correct Model class has been created based on the input.
The abstract base class for models is intended to help implement new Model types (such as s3) - so its clear which methods a model needs to have and which might already be provided by the
Model
implementation.Summary by Sourcery
Introduces a model factory to create model instances, and an abstract base class for models. Adds unit tests for the new model factory. Updates CI to run unit tests.
Enhancements:
ModelFactory
class to encapsulate the logic for creating model instances based on the provided model string and transport configuration.ModelBase
class to define the interface for model implementations, ensuring consistency across different model types.CI:
Tests:
ModelFactory
to verify the correct model class is instantiated based on the input.