LLaVA-NeXT icon indicating copy to clipboard operation
LLaVA-NeXT copied to clipboard

请求基于Mistral-7B-Instruct-v0.3版本的llava-next

Open iMountTai opened this issue 1 year ago • 13 comments

同上。

iMountTai avatar May 28 '24 01:05 iMountTai

Does this also work on NixOS: https://nixos.org/ ?

peaklabs-dev avatar Jun 28 '24 09:06 peaklabs-dev

The install steps above actually can break MicroOS - you don't want to be changing the release as it can cause parts of the system that differentiate between Tumbleweed and MicroOS to misbehave - so we definitely want to specify more aliases above. You also don't need to set up the docker group, as the intent is generally to just have a root user if you're running it on a server, so there's not a lot of point. Just needs to be tweaked a little.

That being said, at least with a quick glance over it, this seems to cover most of what is needed minus documentation and some additional changes I'd suggest adding to install.sh (which I have listed at the end)

My own testing steps - these are not ideal either, but they work for now:

# Set up system - install docker and nano (or your editor of choice) + coolify dependencies, remove podman if installed (not required but having 2 runtimes makes things fight sometimes - ideally we get native podman support at some point) as well as other dependencies
transactional-update run sh -c 'zypper ref && zypper dup -y && zypper in -y docker nano curl wget git jq && zypper rm -y podman'
# Reboot into new OS snapshot
reboot

# Enable docker
systemctl enable --now docker
# Set up SSH stuff
echo -e '# Coolify requires root login over SSH to manage this server\nPermitRootLogin yes' > /etc/ssh/sshd_config.d/70-coolify.conf

# Clone the repo
git clone https://github.com/LEstradioto/coolify.git -b Feat--Customize-Install-Directory
cd coolify
nano scripts/install.sh
# Comment out the zypper-related lines as mentioned
# Add opensuse-microos as a supported OS in the $OS_TYPE check for the support list - this should be fixed in this PR, see below for suggestions

# Start it up!
mkdir /var/coolify
COOLIFY_ROOT_PATH=/var/coolify ./scripts/install.sh

It still references /data for some stuff here, but this is an issue of it still pulling from the CDN - as a result, I could not test end to end, as it'd require modifying the install script substantially, and I don't have time to spend on testing to that degree right now.

A few suggested improvements for @LEstradioto:

  • Add a flag or variable that allows skipping of things like dependency installation and platform checks, maybe ALLOW_UNSUPPORTED_PLATFORMS=1 or something, which instead prints a warning and says "Hey if these are missing this may fail, be sure these are available" - a trapdoor for those who are doing funky stuff is always appreciated :heart:
  • Optionally: add explicit support for opensuse-microos in all areas - if it's microos, add these to the script:

    Note, this may be a bad idea unless MicroOS is a platform that support should be dedicated to going forward - it's fairly niche, so I'd just add the trapdoor and call it a day, rather than adding explicit support.

    transactional-update pkg in -y curl wget git jq
    transactional-update apply
    
  • Add the SSH lines to /etc/ssh/sshd_config.d/70-coolify.conf on all platforms - all major distributions support reading from this path in their default installs, so there's no reason to modify the primary sshd_config file when we can have overrides written here.
  • Move or change the writable path check - this will always fail in its current state, as a directory not existing (which it won't at first in most cases) will fail this check. Could also just check the exit code of mkdir -p "$COOLIFY_ROOT_PATH" to the if check, as it'll return OK even if it exists already - it'll only fail if it's not writable.

If the user doesn't have docker installed, this should probably be managed by the user, not by the install script. Additionally, the install of curl + wget + git + jq should probably be in a toolbox container or something (MicroOS's fully mutable temporary environment for doing things like this) but it seems to only support docker as a runtime when run with -S (sandboxed), which prevents it being used for this purpose.

With the above changes, it should make the installation a little smoother for people on MicroOS (and potentially other systems too re: the config file change for sshd)

TapGhoul avatar Jun 28 '24 16:06 TapGhoul

@ayntk-ai While the patchset could help with NixOS packaging, due to the complexity of the change supporting NixOS may be out of scope of the bounty I have put on this issue. The only real key change to this is finishing the prior work on allowing a custom host installation directory. This is by no means a "Support all immutable OSes" bounty, just a key part in making support possible at all. The average user doing curl file.sh | bash is not the average user using things like MicroOS or NixOS, so I wouldn't expect native support.

TapGhoul avatar Jun 28 '24 16:06 TapGhoul

Thanks for the informations.

The error you got after successful install.sh is because code changes must reflect on Prod. That should succeed with no worries if merged. For now, I think you could clone my repo and run like this:

cp scripts/install.sh install.sh
cp scripts/upgrade.sh upgrade.sh
sed -i 's/^CDN=.*/CDN="."/g' install.sh
sed -i 's/^CDN=.*/CDN="."/g' upgrade.sh
COOLIFY_ROOT_PATH=/var/coolify ALLOW_UNSUPPORTED_PLATFORMS=1 SKIP_DEPENDENCY_INSTALLATION=1 ./install.sh 4.0.0-beta.307

Ive already did a check on the writable of root path, so it fails if no write permission

We could add opensuse-microos. @Silvea12 could help me adding aliases (perhaps check ID_LIKE too) But, I agree that for any explicit support we should wait @andrasbacsai position

Changes made:

  • Refactor encapsulation to install.sh
  • Added new flags, ALLOW_UNSUPPORTED_PLATFORMS, SKIP_DEPENDENCY_INSTALLATION
  • About ssh lines, actually install.sh do not change the PermitRootLogin, so I just updated the warning message suggesting the override on /etc/ssh/sshd_config.d/70-coolify.conf

LEstradioto avatar Jun 28 '24 22:06 LEstradioto

I did a refactor on install.sh too and added Logo and Colors 😄 @andrasbacsai

LEstradioto avatar Jun 28 '24 22:06 LEstradioto

Due to the sheer number of changes in the second commit, I'm hesitant to even test this properly to see if it passes the needs of the original issue, as I may be testing code paths that may be canned. @andrasbacsai can you comment on this change? Not really sure what to do here, the second commit to this PR makes a lot of behavioral changes. I'm going to put this on ice on my end and wait for a comment back from you.

Worth a note too, your mentioned instructions don't work, because changing the CDN URL to pull from the local dir doesn't work when fed into curl.

TapGhoul avatar Jun 29 '24 16:06 TapGhoul

Sorry guys.

I will turn this into Draft. I did more stuff then the requested, my bad. The correct is to separate these in another PRs. So I will do this first

LEstradioto avatar Jun 29 '24 20:06 LEstradioto

Okay, that's it. I undo the last commit.

The command now is like this: COOLIFY_ROOT_PATH=/another/path SKIP_OS=1 sudo -E scripts/install.sh

Only one flag now, just SKIP_OS (this skips os check and dependencies installs). As there are very few cases where using them separately or individually is necessary.

LEstradioto avatar Jul 01 '24 20:07 LEstradioto

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them. While these secrets were previously flagged, we no longer have a reference to the specific commits where they were detected. Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately. Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

gitguardian[bot] avatar Jul 01 '24 21:07 gitguardian[bot]

Aside from $SKIP_OS being an unintuitive flag name to me (but that's personal preference), LGTM? I'm happy to pay out for this on merge by maintainers (once they run their own review)

TapGhoul avatar Jul 04 '24 03:07 TapGhoul

Is there any plan to go forward with this @andrasbacsai? I cannot pay out any bounty until this is sorted.

TapGhoul avatar Aug 09 '24 12:08 TapGhoul

A lot of things change since then, so this needs to be reviewed @LEstradioto (sorry about that).

andrasbacsai avatar Sep 30 '24 13:09 andrasbacsai

Okay, rebased to next and kept variable named BASE_CONFIG_PATH, so no breaking changes, if any

User can opt-in BASE_CONFIG_PATH and MANUAL_DEPENDENCIES

Installing like this: curl -fsSL https://cdn.coollabs.io/coolify/install.sh | env BASE_CONFIG_PATH=/custom/path/to/coolify MANUAL_DEPENDENCIES=true bash

Would install Coolify with a customized installation path and skip automatic dependencies installation. Probably must update docs to Advanced Installation or something

But, hey @andrasbacsai :D! I do have a doubt in line 513 and 516

Every path on codebase was /data/coolify/ and I have no idea why its /_data/coolify/ and /_data/databases/ in those lines

Even though its only in dev, how to match using the right config?

LEstradioto avatar Oct 05 '24 01:10 LEstradioto

@LEstradioto Are you still working on this, if you do not have time I totally understand. Please let me know so I can close this, if not you can always open a new PR in the future.

peaklabs-dev avatar Dec 16 '24 14:12 peaklabs-dev

I'm still down to pay out the bounty when merged, by the way.

TapGhoul avatar Dec 18 '24 18:12 TapGhoul

@LEstradioto Are you still working on this, if you do not have time I totally understand. Please let me know so I can close this, if not you can always open a new PR in the future.

Hey @peaklabs-dev ! Actually it is done. Ive synced just now to next.

Note: In dev mode, altering the installation directory may cause issues (there are lines with hardcoded paths such as lines 513 and 516). Since only in dev, I wouldnt consider an issue?

LEstradioto avatar Dec 18 '24 19:12 LEstradioto

Any update on this?

RiccardoM avatar Mar 03 '25 18:03 RiccardoM

I will close this PR for now, lots of things changed since then. If we (or anyone) have capacity to work on this, this would be a good base.

andrasbacsai avatar Mar 12 '25 13:03 andrasbacsai

Im still up to this! I'll review the new changes next week or so. I've just gone through it and didn't see many changes in the installation scripts, but I'll probably need to update some path variables across the codebase again, and test it. It won't take much time.

LEstradioto avatar Mar 12 '25 15:03 LEstradioto

Thank you so much for building this!!

nikodunk avatar Apr 05 '25 04:04 nikodunk

Update is complete and working in dev. install.sh script is pending testing, Im having VMs issues to proper test. If someone could test, great! Otherwise, I'll follow up later. Please wait for confirmation before moving forward.

LEstradioto avatar May 08 '25 02:05 LEstradioto

Wait, why was this closed? Was it finished? Even if it's not getting merged I'm down to pay out the bounty on it, I'm just not really sure what's going on with this.

TapGhoul avatar May 09 '25 06:05 TapGhoul

It was a mistake, sorry. I recreated the next branch and it auto-closed this PR.

andrasbacsai avatar May 09 '25 10:05 andrasbacsai

Ok it is done. Ive just tested everything, it should be good. Just a question, should I patch the install-1.6.sh and install.1.7.sh too? What are those actually?

LEstradioto avatar May 09 '25 15:05 LEstradioto

@LEstradioto The versioned install scripts are older versions if we need to rollback.

As this is a major change I think we will not add this until v5 (couple of months). So for v5 I think we can provide the ability to change the install dir directly from the start. This will also help us with the upgrade process from v4 to v5 as we will not have to account for custom folder structures.

peaklabs-dev avatar May 13 '25 18:05 peaklabs-dev

Ok. I understand. How you thinking of implementing this at V5? Do you want me to generate a bash script from start over and add to V5? Just let me know!

Just a heads up, if keeping the bash, a better rewrote would follow the structure of my last refactoring, at least the main() block

LEstradioto avatar May 13 '25 18:05 LEstradioto

Not sure yet. v5 will probably use go for the install script as it is getting quite large and go is probably better suited for such a task. If we decided in the end to use bash we will come back to this PR. For now I will close it.

peaklabs-dev avatar May 19 '25 12:05 peaklabs-dev

Okay, since you probably make this happening in near future, makes sense to close and Im good without the Bounty (thanks @TapGhoul for the incentive). Thumbs up for Go choice !

LEstradioto avatar May 19 '25 13:05 LEstradioto