Move common Registry types to MLXLMCommon #229
Conversation
If you want to create a group for the model/registry pieces, please do. |
| public init(creators: [String: @Sendable (URL) throws -> any LanguageModel]) { | ||
| self.creators = creators | ||
| } | ||
| public class LLMTypeRegistry: ModelTypeRegistry, @unchecked Sendable { |
There was a problem hiding this comment.
Changing the name here is probably OK -- callers couldn't really do anything directly with the type before and would have pulled it off the ModelRegistry
| public init(modelConfigurations: [ModelConfiguration]) { | ||
| self.registry = Dictionary(uniqueKeysWithValues: modelConfigurations.map { ($0.name, $0) }) | ||
| } | ||
| public class LLMRegistry: ModelRegistry, @unchecked Sendable { |
There was a problem hiding this comment.
I think we need to keep this the same name -- code probably has reference to MLXLLM.ModelRegistry to get at the static members, e.g. codeLlama13b4bit.
Perhaps the parent could be AbstractModelRegistry?
There was a problem hiding this comment.
Actually, this name change is intentional. I am aware that this would be a breaking change but I believe it will make the code more descriptive. However, in this case, we should inform the developers that ModelRegistry is now an abstract class and must be subclassed to be used.
So, we can:
- Keep the subclasses but rename the parent classes as
AbstractModelRegistryand so on. (My suggestion) - Keep the previous code and rename the parent classes as
AbstractModelRegistryand so on. (Your suggestion)
In my suggestion, it would completely break the existing code but do you agree it's worth it? Because if we make this change, MLXLLM and MLXVLM won't conflict, and the code would be more descriptive when these packages are used IMO. Also, since there has been no release containing MLXVLM so far, this change may be acceptable.
In the end, I am still unsure and looking for your opinion.
This PR moves the common Registry types to MLXLMCommon to avoid code duplication.
Should we reorganize
MLXLMCommondirectory? at least for the registry types. I created three new files in it and the directory looks like a bit messy IMO.I misspelled the class name in the first commit message, sorry