Support platforms without upstream ccache/sccache releases#439
Conversation
|
I verified it works at https://github.com/riseproject-dev/llama.cpp/actions/runs/24143385497/job/70475993840 |
|
It's been merged in |
|
@hendrikmuhs would it please be possible to have a review? Happy to modify in any way! Thank you 🙏 |
| if (method === INSTALL_METHOD.DETECT && await io.which(variant)) { | ||
| // If the install method is "detect" and the package is already installed, | ||
| // don't try to install it. This is to bypass architectures that are not | ||
| // released in upstream projects (linux-riscv64 for ccache/ccache for example). | ||
| } else { | ||
| const pkg = selectPackage(variant) | ||
| await pkg.install(method); | ||
| } |
There was a problem hiding this comment.
This is already handled automatically by the install function:
async install(method: INSTALL_METHOD): Promise<void> {
switch (method) {
// ...
case INSTALL_METHOD.DETECT:
if (!await io.which(this.variant)) await this.installAuto()
break
// ...
}
}There was a problem hiding this comment.
Oh, I just realized this can cause issues if the specified combination doesn't exist... but that will throw anyways in selectPackage:
export function selectPackage(variant: VARIANT): Package {
// ...
if (!entry) {
throw new Error(
`No metadata found for combination: variant=${variant}, arch=${archKey}, platform=${platform}`
);
}
}
There was a problem hiding this comment.
🤔 The flow is somehow strange now, as we bypass the usual logic of 1. getting the package and 2. call install on the returned package.
@crueter would it make sense to relax package to never throw and instead return an UnknownPackage, which could be a constant(or a package with Unknown's). Instead of selectPackage we could let install handle the case when INSTALL_METHOD != DETECT and package == UnknownPackage.
(FWIW: It makes total sense to me to not restrict the action in case of unknown platform/package and let users install by themselves.)
There was a problem hiding this comment.
I'll update to that logic.
There was a problem hiding this comment.
I updated it, let me know what you think, and if it's in line with what you had in mind.
There was a problem hiding this comment.
looks good, @crueter, you wrote the original implementation, wdyt?
There was a problem hiding this comment.
As stated in my other review: not "ideal" but plenty fine for now. I'll work on implementing something a bit better somewhat soon. LGTM
luhenry
force-pushed
the
main
branch
2 times, most recently
from
55248ef to
944c4c8
Compare
luhenry
changed the title
Introduce UnknownPackage to handle platform/arch combinations (e.g. linux-riscv64) that lack upstream binary releases. When install-method is "detect", fall back to locally installed ccache/sccache instead of failing. Move the detect-and-skip logic from runInner into UnknownPackage.install so all install paths go through the same Package interface.
|
Testing it out at https://github.com/luhenry/llama.cpp/actions/runs/24450526137/job/71438158660. Example output: |
| export class UnknownPackage { | ||
| constructor( | ||
| public readonly variant: VARIANT, | ||
| public readonly arch: ARCH, | ||
| public readonly platform: PLATFORM) { } | ||
|
|
||
| async install(method: INSTALL_METHOD): Promise<void> { | ||
| if (method === INSTALL_METHOD.DETECT && await io.which(this.variant)) { | ||
| // If the install method is "detect" and the package is already installed, | ||
| // don't try to install it. This is to bypass architectures that are not | ||
| // released in upstream projects (linux-riscv64 for ccache/ccache for example). | ||
| } else { | ||
| throw new Error( | ||
| `No metadata found for combination: variant=${this.variant}, arch=${this.arch}, platform=${this.platform}` | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Honestly, this isn't my favorite way to implement this, but it works. I'll devise something a bit "cleaner" at some point, but LGTM for now
|
Is there anything else you'd like me to change for it to go forward? Thank you! |
|
All good on my end, Hendrik is probably just busy so wait for now. Thanks for the change! |
|
Sorry, was about to merge, but got distracted with something else. Thanks for the contribution! |
3 participants
Hello! This is an issue that we ran into in
ggml-org/llama.cpp. This should allow to support more platforms for which there are ccache/sccache binaries available in their respective distributions, but not yet in https://github.com/ccache/ccache releases