Cleanup: remove deprecated imports, add LocalizedError, use async URLSession - #1
Conversation
…Session - Remove standalone `import MLXRandom` and `import MLXFast` where namespaced calls work via `import MLX` - Add LocalizedError conformance with errorDescription to all public error enums for better consumer diagnostics - Replace DispatchSemaphore + callback-based URLSession.dataTask with modern async URLSession.data(for:) in fetchRemoteData() - Hoist async image loading out of Device.withDefaultDevice closure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010ca03c26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let loadStart = clock.now | ||
| var loaded: [ConditioningImage] = [] | ||
| for spec in imageSpecs { | ||
| try await loaded.append(loadConditioningImage(spec: spec)) | ||
| } |
There was a problem hiding this comment.
Here loadConditioningImage is awaited before entering Device.withDefaultDevice(.gpu). That helper builds an MLXArray from the decoded image (see CLI+Image.loadConditioningImage), so this change creates the conditioning tensors on the default device (typically CPU) instead of GPU. When the pipeline later consumes conditioningImages inside the GPU context, MLX has to transfer those arrays, which is an unintended host↔device copy and a measurable performance regression for image conditioning. Consider moving the loading (or at least the MLXArray creation) back inside the GPU block or explicitly placing the array on the GPU.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This suggestion is incorrect for MLX on Apple Silicon.
Apple Silicon uses unified memory (UMA) — the GPU and CPU share the same physical memory space. There is no separate "host" and "device" memory, so there is no "host↔device copy" like you'd see with CUDA on discrete NVIDIA GPUs. An MLXArray created outside Device.withDefaultDevice(.gpu) is already accessible to the GPU without any data transfer.
Device.withDefaultDevice(.gpu) sets the compute context (where operations execute), not where data lives. The conditioning image arrays are read directly by GPU kernels inside the pipeline with zero copy overhead.
Additionally, loadConditioningImage is async (it uses URLSession.data), so it cannot be called inside Device.withDefaultDevice(.gpu), which takes a synchronous closure. The current code correctly hoists the async work before the device block — this is the intended pattern.
There was a problem hiding this comment.
human Ronald here. Can I trigger your @codex as well?
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
There was a problem hiding this comment.
I see. I need to bring my own Codex? That's going to be fun. We'll have three bots talking to each other
There was a problem hiding this comment.
I will pull down the PR on my local machine and testing it late tonight, currently the Mac is occupied by porting glm ocr:)
There was a problem hiding this comment.
@codex look at the previous comments and give me your take
There was a problem hiding this comment.
I just connected Claude to Github too. Let's see if I can summon @claude to turn this discussion into a Furby Frenzy
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010ca03c26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let semaphore = DispatchSemaphore(value: 0) | ||
| var result: Result<Data, Error>? | ||
| let (data, response) = try await session.data(for: request) |
There was a problem hiding this comment.
When session.data(for:) throws (for example on DNS/TLS failures, offline mode, or request timeout), this now propagates a raw URLError instead of the CLI-specific CLIError.invalidOption that previously included the image URL and a clear "Failed to download image" message. In generate --image ... flows that fetch remote conditioning images, users will now get less actionable errors and lose consistent CLI error formatting; wrapping this call in do/catch and rethrowing CLIError.invalidOption(...) restores the prior behavior.
Useful? React with 👍 / 👎.
|
@ronaldmannak thanks for cleanup :) |
2 participants
Summary
@preconcurrency importstatements that are no longer neededLocalizedErrorconformance to error types for better error descriptionsData(contentsOf:)with asyncURLSession.shared.data(from:)for image loadingThese are independent cleanup improvements that don't change any behavior.
Files changed
Sources/Flux2/PixtralVisionTower.swiftSources/Flux2/VAEBlocks.swiftSources/Flux2/Flux2DevPipeline.swiftSources/Flux2/Flux2KleinPipeline.swiftSources/Flux2/LatentPreparation.swiftSources/Flux2/Scheduler.swiftSources/Flux2CLI/CLI+Generate.swiftSources/Flux2CLI/CLI+Image.swiftTest plan
swift build🤖 Generated with Claude Code