add a setup ilab script for macos#2419
Conversation
d06ebcf to
20dc583
Compare
|
hi, please someone help to take a look |
cdoern
left a comment
There was a problem hiding this comment.
can you add some tests which run this? This is great! just a few comments on what we do/do not support
scripts/setup-ilab-macos.sh
Outdated
|
|
||
| # Clean llama_cpp_python cache and install with MPS support | ||
| pip cache remove llama_cpp_python | ||
| pip install 'instructlab[mps]' |
There was a problem hiding this comment.
I don't know if this target is actively maintained, but it should be if it isn't
There was a problem hiding this comment.
again, I don't think [mps] is the right target, this tends to install an older ilab version. This should probably just be pip install 'instructlab'
scripts/setup-ilab-macos.sh
Outdated
| # Intel Mac | ||
| echo "Intel Mac detected. Installing InstructLab with Intel CPU..." | ||
| CMAKE_ARGS="-DLLAMA_METAL=off" pip install instructlab | ||
| echo "InstructLab installed with Intel CPU support." |
There was a problem hiding this comment.
we don't technically support Intel Mac, so I am wary to put this into our codebase
20dc583 to
39d9d5c
Compare
84db2c0 to
d9ff33d
Compare
|
someone else help to take a look? +2 |
cdoern
left a comment
There was a problem hiding this comment.
tests look nice, but the build target still is pointing to MPS! Please take a look at those targets and correct me if I am wrong but I think that one is out of date
scripts/setup-ilab-macos.sh
Outdated
|
|
||
| # Clean llama_cpp_python cache and install with MPS support | ||
| pip cache remove llama_cpp_python | ||
| pip install 'instructlab[mps]' |
There was a problem hiding this comment.
again, I don't think [mps] is the right target, this tends to install an older ilab version. This should probably just be pip install 'instructlab'
|
This pull request has merge conflicts that must be resolved before it can be |
d9ff33d to
1b0a95a
Compare
1b0a95a to
c4ad772
Compare
c4ad772 to
07071c2
Compare
Signed-off-by: reid_liu <guliu@redhat.com>
7a454e8 to
4e9517e
Compare
|
I think this is cool. I'm currently working on something in the same realm for MacOS, in the form of an intermode mode versus a script. I'm hoping to share within the coming weeks |
|
I think this can exist with an interactive mode, they serve kind of different use cases. |
cdoern
left a comment
There was a problem hiding this comment.
lgtm. @nathan-weinberg PTAL
| OS_TYPE=$(uname) | ||
| OS_ARCH=$(uname -m) | ||
| CPU_BRAND=$(sysctl -n machdep.cpu.brand_string) | ||
| LINK_TARGET_DIR="/usr/local/bin" |
There was a problem hiding this comment.
It gives me an error when I launch it, do I need to run it with sudo?
Error: No write permission to /usr/local/bin.
Run the script with sudo or choose another directory.
There was a problem hiding this comment.
Thanks for you try. I think you may need to use sudo for the soft link.
There was a problem hiding this comment.
weird, only show two lines? Should be three
echo -e "\nError: No write permission to $LINK_TARGET_DIR."
echo "Run the script with sudo or choose another directory."
echo -e "Alternatively, you can use sudo create the symlink manually for $INSTALL_DIR/venv/bin/ilab"
There was a problem hiding this comment.
Of course I can use sudo, as you say, but when I run the full unit test suite with tox -e py3-unit it also runs this test which fails for the same reason. Maybe we can remove it from the pytest suite? (easiest way is to rename it to something like macos_install.py)
There was a problem hiding this comment.
Thank you so much for feedback. But based on the previous discussion, the test is needed for the script.
I try to fix with the venv whole path(will not exit) to run the next steps if no permission, make sure it can finish to the end. I also tried simply use sudo with curl command, seems will totally change the dir permission, that's not good to the current user.
can you try below?
2827
|
Is there a reason why we wouldn't just run |
When users first interact with it, it's important to let them know what they are configuring, what settings are being applied, and the paths where configurations are stored. Providing this information helps users understand the setup process and be aware of what changes have been made. Some configurations allow users to customize their settings rather than sticking with the default ones. However, the --non-interactive option can still be used to quickly set up the configuration file without requiring user interaction if you want a more automated setup. |
Some users might need `sudo` to set the soft link, so it will fail after install. To make it fluently, it will use `the whole path` to run the next steps. #2419 (comment) **Checklist:** - [ ] **Commit Message Formatting**: Commit titles and messages follow guidelines in the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary). - [ ] [Changelog](https://github.com/instructlab/instructlab/blob/main/CHANGELOG.md) updated with breaking and/or notable changes for the next minor release. - [ ] Documentation has been updated, if necessary. - [ ] Unit tests have been added, if necessary. - [ ] Functional tests have been added, if necessary. - [ ] E2E Workflow tests have been added, if necessary. Approved-by: nathan-weinberg Approved-by: jaideepr97
Due to macOS is one of the more popular user bases, and deployment instructions(warnings, notes) can become increasingly detailed, so providing a setup script will help users get started quickly.
Checklist:
conventional commits.