refactoring - #176
Conversation
![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: 4bd2761b2d
ℹ️ 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".
| lock (Gate) | ||
| { | ||
| if (_builtInsLoaded) | ||
| return; | ||
| _builtInsLoaded = true; | ||
| } |
There was a problem hiding this comment.
When two threads first access TryGet, Resolve, or All, the first thread sets _builtInsLoaded and releases Gate before registering the descriptors, so the second thread returns from EnsureBuiltIns and observes an empty or partially populated registry. A concurrent load of a supported model can therefore throw NotSupportedException; keep initialization under the reentrant monitor or use an initialization state that makes other threads wait for completion.
Useful? React with 👍 / 👎.
| // Ordered so the pick is stable when several companions sit side by side. | ||
| string[] matches = System.IO.Directory.GetFiles(directory, hint); | ||
| if (matches.Length > 0) |
There was a problem hiding this comment.
On case-sensitive filesystems, Directory.GetFiles(directory, hint) applies the casing from the descriptor literally. For example, both Nemotron hints contain uppercase Nemotron, so a valid companion such as nemotron-mmproj.gguf is no longer auto-loaded, whereas the replaced CLI code filtered Nemotron names with OrdinalIgnoreCase; enumerate candidates and match hints case-insensitively to preserve that behavior.
Useful? React with 👍 / 👎.
| // Audio path: the chat template emits a `<so_embedding>` per uploaded | ||
| // audio file so the model "sees" the modality, but real inference is | ||
| // gated on a Parakeet audio mmproj that this distribution does not ship. | ||
| // The test data still gets preprocessed in the CLI for verification. | ||
| // The clip is still decoded and turned into its log-mel spectrogram so | ||
| // the frontend is exercised and the operator is told, once, why no audio | ||
| // inference will happen. (This used to live in the CLI, which meant the | ||
| // server silently ignored audio entirely; it belongs with the rest of | ||
| // the architecture's media handling.) | ||
| if (message.AudioPaths != null && message.AudioPaths.Count > 0) | ||
| PreprocessNemotronAudioForVerification(message.AudioPaths[0]); |
There was a problem hiding this comment.
For an audio-only Nemotron request without a loaded vision projector, the method returns at line 647 before reaching this newly centralized audio handling. The CLI now treats Nemotron as IAudioCapableModel, so it also skips the generic unsupported-audio warning, leaving the <so_embedding> prompt and audio silently unused instead of decoding it and emitting the explicit warning that the previous CLI path provided; move audio handling before the vision-only early return.
Useful? React with 👍 / 👎.
Engine comparison — TensorSharp vs llama.cpp (PR smoke)No report artifact was produced — the benchmark failed before generating results (see the workflow logs). |
1 participant
No description provided.