Comment by danielhanchen

2 days ago

Oh hey I'm assuming this is for conversion to GGUF after a finetune? If you need to quantize to GGUF Q4_K_M, we have to compile llama.cpp, hence apt-get and compiling llama.cpp within a Python shell.

There is a way to convert to Q8_0, BF16, F16 without compiling llama.cpp, and it's enabled if you use `FastModel` and not on `FastLanguageModel`

Essentially I try to do `sudo apt-get` if it fails then `apt-get` and if all fails, it just fails. We need `build-essential cmake curl libcurl4-openssl-dev`

See https://github.com/unslothai/unsloth-zoo/blob/main/unsloth_z...

66 comments

danielhanchen

Reply
pxc  2 days ago

It seems Unsloth is useful and popular, and you seem responsive and helpful. I'd be down to try to improve this and maybe package Unsloth for Nix as well, if you're up for reviewing and answering questions; seems fun.

Imo it's best to just depend on the required fork of llama.cpp at build time (or not) according to some configuration. Installing things at runtime is nuts (especially if it means modifying the existing install path). But if you don't want to do that, I think this would also be an improvement:

  - see if llama.cpp is on the PATH and already has the requisite features
  - if not, check /etc/os-release to determine distro
  - if unavailable, guess distro class based on the presence of high-level package managers (apt, dnf, yum, zypper, pacman) on the PATH
  - bail, explain the problem to the user, give copy/paste-friendly instructions at the end of we managed to figure out where we're running

Is either sort of change potentially agreeable enough that you'd be happy to review it?

  • danielhanchen  2 days ago

    As an update, I pushed https://github.com/unslothai/unsloth-zoo/commit/ae675a0a2d20...

    (1) Removed and disabled sudo

    (2) Installing via apt-get will ask user's input() for permission

    (3) Added an error if failed llama.cpp and provides instructions to manual compile llama.cpp

    • mFixman  2 days ago

      Maybe it's a personal preference, but I don't want external programs to ever touch my package manager, even with permission. Besides, this will fail loudly for systems that don't use `apt-get`.

      I would just ask the user to install the package, and _maybe_ show the command line to install it (but never run it).

      7 replies →

  • danielhanchen  2 days ago

    Thanks for the suggestions! Apologies again I'm pretty bad at packaging, so hence the current setup.

    1. So I added a `check_llama_cpp` which checks if llama.cpp does exist and it'll use the prebuilt one https://github.com/unslothai/unsloth-zoo/blob/main/unsloth_z...

    2. Yes I like the idea of determining distro

    3. Agreed on bailing - I was also thinking if doing a Python input() with a 30 second waiting period for apt-get if that's ok? We tell the user we will apt-get some packages (only if apt exists) (no sudo), and after 30 seconds, it'll just error out

    4. I will remove sudo immediately (ie now), and temporarily just do (3)

    But more than happy to fix this asap - again sorry on me being dumb

    • mkl  2 days ago

      It shouldn't install any packages itself. Just print out a message about the missing packages and your guess of the command to install them, then exit. That way users can run the command themselves if it's appropriate or add the packages to their container build or whatever. People set up machines in a lot of different ways, and automatically installing things is going to mess that up.

      11 replies →

Balinares  2 days ago

I'll venture that whoever is going to fine-tune their own models probably already has llama.cpp installed somewhere, or can install if required.

Please, please, never silently attempt to mutate the state of my machine, that is not a good practice at all and will break things more often than it will help because you don't know how the machine is set up in the first place.

  • danielhanchen  2 days ago

    Oh yes so before we install llama.cpp we do an path environment check and if its not defined then it'll install.

    But yes agreed there won't be any more random package installs sorry!

elteto  2 days ago

Dude, this is NEVER ok. What in the world??? A third party LIBRARY running sudo commands? That’s just insane.

You just fail and print a nice error message telling the user exactly what they need to do, including the exact apt command or whatever that they need to run.

  • danielhanchen  2 days ago

    Yes I had that at the start, but people kept complaining they don't know how to actually run terminal commands, hence the shortcut :(

    I was thinking if I can do it during the pip install or via setup.py which will do the apt-get instead.

    As a fallback, I'll probably for now remove shell executions and just warn the user

    • rfoo  2 days ago

      IMO the correct thing to do to make these people happy, while being sane, is - do not build llama.cpp on their system. Instead, bundle a portable llama.cpp binary along with unsloth, so that when they install unsloth with `pip` (or `uv`) they get it.

      Some people may prefer using whatever llama.cpp in $PATH, it's okay to support that, though I'd say doing so may lead to more confused noob users spam - they may just have an outdated version lurking in $PATH.

      Doing so makes unsloth wheel platform-dependent, if this is too much of a burden, then maybe you can just package llama.cpp binary and have it on PyPI, like how scipy guys maintain a https://pypi.org/project/cmake/ on PyPI (yes, you can `pip install cmake`), and then depends on it (maybe in an optional group, I see you already have a lot due to cuda shit).

      9 replies →

  • lyu07282  2 days ago

    on a meta level its kind of worrying for the ecosystem that there is nothing in PyPI that blocks & bans developers who try to run sudo on setup. I get they don't have the resources to do manual checks, but literally no checks against malicious packages?

    • danielhanchen  2 days ago

      Sadly not - you can run anything within a python shell since there's os system, subprocess popen and exec - it's actually very common for setup.py files where installers execute commands

      But I do agree maybe for better security pypi should check for commands and warn

pshirshov  2 days ago

It won't work well if you deal with non ubuntu+cuda combination. Better just fail with a reasonable message.

lambda  2 days ago

[flagged]

  • danielhanchen  2 days ago

    I added it since many people who used Unsloth don't know how to compile llama.cpp, so the only way from Python's side is to either (1) Install it via apt-get within the Python shell (2) Error out then tell the user to install it first, then continue again

    I chose (1) since it was mainly for ease of use for the user - but I agree it's not a good idea sorry!

    :( I also added a section to manually compile llama.cpp here: https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-...

    But I agree I should remove apt-gets - will do this asap! Thanks for the suggestions :)

    • Imustaskforhelp  2 days ago

      Hey man, I was seeing your comments and you do seem to respond to each and everyone nicely regarding this sudo shenanigan.

      I think that you have removed sudo so this is nice, my suggestion is pretty similar to that of pxc (basically determine different distros and use them as that)

      I wonder if we will ever get a working universal package manager in linux, to me flatpak genuinely makes the most sense even sometimes for cli but flatpak isn't built for cli unlike snap which both support cli and gui but snap is proprietory.

      3 replies →

  • pxx  2 days ago

    > What this whole thing hallucinated by an LLM?

    probably not, because LLMs are a little more competent than this