fix: inject the HubApi #209
Conversation
|
Can you explain more what the intent is here? I see some code to give custom Progress for the different parts of loading the model -- does this override the download progress? I am not sure if these nest properly because it isn't set as the "current" I am not sure what is meant by injecting Hub -- it is already taken as a parameter. Thanks! |
|
@davidkoski It is a small change to better track loading progress and to support local URLs. The PR doesn't override the download progress - it includes it as part of a larger progress structure, with the download being the first 30% of the total progress if I am not mistaken. The approach should nest properly as it's creating a new Progress object with a total unit count of 100 and updating it as each phase completes. The "injecting Hub" part refers to ensuring the HubApi instance is properly passed through to all the necessary functions that need it, particularly in cases like the StableDiffusion module where it was inconsistently passed. |
| /// - ``presetSDXLTurbo`` | ||
| /// - ``presetStableDiffusion21Base`` | ||
| /// - presetSDXLTurbo | ||
| /// - presetStableDiffusion21Base |
There was a problem hiding this comment.
Why remove the backquotes? With them we get links to the symbols
Got it -- I just ran with a download and could see the progress going. |
| public static let shared = LLMModelFactory() | ||
|
|
||
| /// registry of model type, e.g. configuration value `llama` -> configuration and init methods |
There was a problem hiding this comment.
Why remove documentation?
|
|
||
| // load the generic config to unerstand which model and how to load the weights |
There was a problem hiding this comment.
I think this comment (minus maybe the typo) is still good to keep around -- the two phase config loading is otherwise a bit mysterious.
| /// ``textToImageGenerator(hub:configuration:)`` or | ||
| /// ``imageToImageGenerator(hub:configuration:)`` to produce the ``ImageGenerator``. | ||
| /// For custom model locations, use: | ||
| /// swift /// let config = StableDiffusionConfiguration.custom( /// baseURL: URL(fileURLWithPath: "/path/to/model"), /// isXL: true // true for SDXL, false for base model /// ) /// |
There was a problem hiding this comment.
This comment looks mangled -- it should probably be something like:
/// ```swift
/// let config ...
/// ```
| @@ -127,9 +127,11 @@ public actor ModelContainer<M> { | |||
|
|
|||
| /// create a ``ModelContainer`` that supports ``TextToImageGenerator`` | |||
| static public func createTextToImageGenerator( | |||
| configuration: StableDiffusionConfiguration, loadConfiguration: LoadConfiguration = .init() | |||
| hub: HubApi = HubApi(), | |||
There was a problem hiding this comment.
I see -- this is the one you mentioned that was missing 👍
Improved Progress Tracking:
The PR adds structured progress reporting with specific percentages for each loading phase (download, config loading, weight loading, tokenizer loading)
Instead of just forwarding the download progress, it breaks down the entire loading process into meaningful stages
For example: 0-30% for download, 30-50% for config loading, 50-80% for weights, and 80-100% for tokenizer
Custom Model Loading Support:
Adds direct support for loading models from custom file paths with the directURL property
This allows loading models from local directories without requiring them to be in the Hub structure
Consistent HubApi Injection:
The PR makes HubApi injection more consistent across different factory methods
Ensures HubApi is passed to all relevant model creation functions