Well, I know I just asked if this was ready for review, but I figured
I'd go ahead and do some "low-hanging fruit review" while I wait for
you to get a chance to respond ;)
Fortunately, most of what I've noticed so far is minor and stylistic.
There are some bigger, discussion-opening comments near the end.
Firstly, TODO comments are conventionally spelled as one word. This is
not grammatically correct, nor is it a requirement, but it is the
convention and folks searching files for such comments will likely
search for "TODO" rather than "TO DO".
Secondly, the synopsis should probably either drop the "Port of" or
change "in" to "to". I don't think this is a major issue; it just
flows better like that imo. I wouldn't oppose merging over this so if
you prefer it the way it is, that's fine.
Next, the description wants expansion. It should be three to five
sentences. The sentence there now is a fine start, though it needs
ending punctuation. Perhaps a sentence explaining how this package can
be used ("whisper-cpp can be integrated with other programs to provide
speech-to-text support" or something) and one explaining what makes it
unique ("because whisper-cpp uses a leading speech recognition model,
it is able to perform speech recognition rapidly with relatively few
resources"). You could perhaps mention that it can run on CPU as well
as GPU, that it offers some integrations with certain hardware, or so
on. Whatever you think is important or interesting; these are just
some ideas :)
Now we enter the discussion-launching comments.
I notice that ggml is vendored. This one is tricky. Firstly, there is
no standalone ggml package (yet; I saw your patch 65284). Usually,
Guix only asks that dependencies be unvendored if there is already a
standalone package for the dependency, so unless and until that is
merged there is no real issue. Secondly, though, and this touches on
the ggml patches, it seems that there are no formal releases of ggml
yet, and development is happening in the repositories for whisper-cpp
and llama-cpp as well as its own repository. It seems these three
versions of ggml are all slightly out of sync with each other. It
would be nice if upstream used git submodules to ensure their work was
synchronized. Alas, we as Guix can't do anything about that, and we
must ensure the packages we offer work correctly. The inconsistencies
between these versions of ggml make me think packaging it separately
would risk breakage. With that in mind, might it be best to drop the
standalone ggml patchset and just let llama-cpp and whisper-cpp vendor
their versions? While suboptimal because it results in building "the
same package" multiple times, I would argue that the divergence in the
code means they are not, in fact, the same package.
Finally, is this package complete? Looking at the store directory for
the package, I see headers and the like but no actual models. Is this
sufficient for using the inference? Are client libraries or programs
supposed to install models themselves? Or can this package be used to
generate models as described in the project's README? If not, should
it be able to? I am admittedly fairly ignorant about the machine
learning ecosystem, so feel free to explain as much as you think may be
necessary. My goal in these questions is ensuring users get what they
expect from this package.
Relatedly, if this package is complete but requires further setup, I
would strongly support explaining that in the package description. As
a user, I've encountered a few packages that require more setup and
don't mention that they do, and I'm then frustrated and confused when I
learn this from trying to use the package and then have no support from
Guix in trying to make things work properly. (Another step beyond this
may be offering a system service which performs configuration, but that
can be a future, separate patch.)
To circle back to what I mentioned in my first email, I would like to
package the whisper.el Emacs mode[1]. Currently, whisper.el plans to
install and compile whisper.cpp and its models itself; I think we as
Guix should make this unnecessary for an imagined future emacs-whisper
package.
Looking forward to hearing from you,
Juli
