Open
Conversation
Contributor
|
Generally a good idea, resolve your conflicts and pass the lint and I will read again |
c567c4e to
7360bd8
Compare
7360bd8 to
7090a6c
Compare
7090a6c to
0417f96
Compare
Author
Now it should be rebased to the new main. All tests seems to pass now |
Owner
|
@JamesNyeVRGuy What do you think? Shall I squash and merge? |
Contributor
There was a problem hiding this comment.
Nice work — the core design is solid and the test coverage is thorough. A few things to address before merge:
Suggestions:
- The branching in
create_engine(backend.py lines ~308-325) is subtle — in the auto/None path, assets are downloaded beforeresolve_backendso that_auto_detect_backendcan find the MLX weights and choose MLX. In the explicit path,resolve_backendruns first. This ordering matters but isn't obvious; a short comment explaining why would help future readers. - The
.to(model_precision)change ininference_engine.pylooks like a separate bug fix. Worth calling out in the PR description or splitting into its own commit so it's easier to bisect later.
Minor observation (non-blocking):
_extract_path_from_outputmatches any line whose path ends in.safetensors— could pick up a stray log line. Probably fine in practice but worth being aware of.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
This change automatically downloads weights of the model if they are not present in the checkpoints folder, removing the need of manual download and setup
How was it tested?
Checked all synthetic tests and manual tests passed
Checklist
uv run pytestpassesuv run ruff checkpassesuv run ruff format --checkpasses