5 ms·

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 com

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_zoo/llama_cpp.py#L298 https://github.com/unslothai/unsloth-zoo/blob/main/unsloth_z...

elteto 1y 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.
- pxc 1y ago
  How unusual is this for the ecosystem?
  - pshirshov 1y ago
    Unfortunately, Python ecosystem is the Wild West.
- danielhanchen 1y 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
  - devin 1y ago
    Don't optimize for these people.
    - danielhanchen 1y ago
      Yep agreed - I primarily thought it was a reasonable "hack", but it's pretty bad security wise, so apologies again. The current solution hopefully is in between - ie sudo is gone, apt-get will run only after the user agrees by pressing enter, and if it fails, it'll tell the user to read docs on installing llama.cpp
      - woile 1y ago
        Don't apologize, you are doing amazing work. I appreciate the effort you put. Usually you don't make assumptions on the host OS, just try to find the things you need and if not, fail, ideally with good feedback. If you want to provide the "hack", you can still do it, but ideally behind a flag, `allow_installation` or something like that. This is, if you want your code to reach broader audiences.
        danielhanchen 1y ago
        Thank you! :)
        shaan7 1y ago
        Yep there's no need to apologize, you've been very courteous and took all that feedback constructively. Good stuff :)
        danielhanchen 1y ago
        Thank you!
      - deleted 1y ago
        [deleted]
  - 1y ago
- danielhanchen 1y ago
  As an update, I pushed https://github.com/unslothai/unsloth-zoo/commit/ae675a0a2d208a20f6dbdccccec0c8ce995fe6ec 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 Again apologies on my dumbness and thanks for pointing it out!
- lyu07282 1y 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 1y 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
lambda 1y ago
[flagged]
- pxx 1y ago
  > What this whole thing hallucinated by an LLM? probably not, because LLMs are a little more competent than this
- danielhanchen 1y 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-do-i-manually-save-to-gguf 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 1y 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.
    - danielhanchen 1y ago
      Hey :) I love suggestions and keep them coming! :) I agree on handling different distros - sadly I'm not familiar with others, so any help would be appreciated! For now I'm most familiar with apt-get, but would 100% want to expand out! Interesting will check flatpak out!
pxc 1y 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 1y 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_zoo/llama_cpp.py#L277 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 1y 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.
pshirshov 1y ago
It won't work well if you deal with non ubuntu+cuda combination. Better just fail with a reasonable message.
- danielhanchen 1y ago
  For now I'm re-directly people to our docs https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-do-i-manually-save-to-gguf https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-... But I'm working on more cross platform docs as well!
  - pshirshov 1y ago
    My current solution is to pack llama.cpp as a custom nix formula (the one in nixpkgs has the conversion script broken) and run it myself. I wasn't able to run unsloth on ROCM nor for inference nor for conversion, sticking with peft for now but I'll attempt again to re-package it.
    - danielhanchen 1y ago
      Oh interesting oh for ROCM there are some installation instructions here: https://rocm.docs.amd.com/projects/ai-developer-hub/en/latest/notebooks/fine_tune/unsloth_Llama3_1_8B_GRPO.html https://rocm.docs.amd.com/projects/ai-developer-hub/en/lates... I'm working with the AMD folks to make the process easier, but it looks like first I have to move off from pyproject.toml to setup.py (allows building binaries)
      - pshirshov 1y ago
        Yes, it's trivial with the pre-built vllm docker, but I need a declarative way to configure my environment. The lack of prebuilt rocm wheels for vllm is the main hindrance for now but I was shocked to see the sudo apt-get in your code. Ideally, llama.cpp should publish their gguf python library and the conversion script to pypi with every release, so you can just add that stuff as a dependency. vllm should start publishing a rocm wheel, after that unsloth would need to start publishing two versions - a cuda one and a rocm one.
Balinares 1y 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 1y 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!
  - Balinares 1y ago
    Thanks for the reply! If I can find the time (that's a pretty big if), I'll try to send a PR to help with the packaging.
    - danielhanchen 1y ago
      No worries :)

elteto 1y 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.

pxc 1y ago
How unusual is this for the ecosystem?
- pshirshov 1y ago
  Unfortunately, Python ecosystem is the Wild West.
danielhanchen 1y 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
- devin 1y ago
  Don't optimize for these people.
  - danielhanchen 1y ago
    Yep agreed - I primarily thought it was a reasonable "hack", but it's pretty bad security wise, so apologies again. The current solution hopefully is in between - ie sudo is gone, apt-get will run only after the user agrees by pressing enter, and if it fails, it'll tell the user to read docs on installing llama.cpp
    - woile 1y ago
      Don't apologize, you are doing amazing work. I appreciate the effort you put. Usually you don't make assumptions on the host OS, just try to find the things you need and if not, fail, ideally with good feedback. If you want to provide the "hack", you can still do it, but ideally behind a flag, `allow_installation` or something like that. This is, if you want your code to reach broader audiences.
      - danielhanchen 1y ago
        Thank you! :)
      - shaan7 1y ago
        Yep there's no need to apologize, you've been very courteous and took all that feedback constructively. Good stuff :)
        danielhanchen 1y ago
        Thank you!
    - deleted 1y ago
      [deleted]
- 1y ago
danielhanchen 1y ago
As an update, I pushed https://github.com/unslothai/unsloth-zoo/commit/ae675a0a2d208a20f6dbdccccec0c8ce995fe6ec 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 Again apologies on my dumbness and thanks for pointing it out!
lyu07282 1y 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 1y 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

lambda 1y ago

[flagged]

pxx 1y ago
> What this whole thing hallucinated by an LLM? probably not, because LLMs are a little more competent than this
danielhanchen 1y 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-do-i-manually-save-to-gguf 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 1y 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.
  - danielhanchen 1y ago
    Hey :) I love suggestions and keep them coming! :) I agree on handling different distros - sadly I'm not familiar with others, so any help would be appreciated! For now I'm most familiar with apt-get, but would 100% want to expand out! Interesting will check flatpak out!

pxc 1y 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 1y 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_zoo/llama_cpp.py#L277 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 1y 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.

pshirshov 1y ago

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

danielhanchen 1y ago
For now I'm re-directly people to our docs https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-do-i-manually-save-to-gguf https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-... But I'm working on more cross platform docs as well!
- pshirshov 1y ago
  My current solution is to pack llama.cpp as a custom nix formula (the one in nixpkgs has the conversion script broken) and run it myself. I wasn't able to run unsloth on ROCM nor for inference nor for conversion, sticking with peft for now but I'll attempt again to re-package it.
  - danielhanchen 1y ago
    Oh interesting oh for ROCM there are some installation instructions here: https://rocm.docs.amd.com/projects/ai-developer-hub/en/latest/notebooks/fine_tune/unsloth_Llama3_1_8B_GRPO.html https://rocm.docs.amd.com/projects/ai-developer-hub/en/lates... I'm working with the AMD folks to make the process easier, but it looks like first I have to move off from pyproject.toml to setup.py (allows building binaries)
    - pshirshov 1y ago
      Yes, it's trivial with the pre-built vllm docker, but I need a declarative way to configure my environment. The lack of prebuilt rocm wheels for vllm is the main hindrance for now but I was shocked to see the sudo apt-get in your code. Ideally, llama.cpp should publish their gguf python library and the conversion script to pypi with every release, so you can just add that stuff as a dependency. vllm should start publishing a rocm wheel, after that unsloth would need to start publishing two versions - a cuda one and a rocm one.

Balinares 1y 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 1y 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!
- Balinares 1y ago
  Thanks for the reply! If I can find the time (that's a pretty big if), I'll try to send a PR to help with the packaging.
  - danielhanchen 1y ago
    No worries :)