ilab wrapper script adjustments
#680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
omertuc
force-pushed
the
wrapper
branch
4 times, most recently
from
7443a07 to
fe80cd3
Compare
omertuc
changed the title
ilab wrapper script adjustmentsilab wrapper script adjustments
omertuc
requested review from
MichaelClifford,
rhatdan,
sallyom,
lmilbaum,
cgwalters and
Gregory-Pereira
as code owners
omertuc
force-pushed
the
wrapper
branch
5 times, most recently
from
4ba8832 to
65fe141
Compare
|
Excellent thank you @omertuc LGTM |
n1hility
approved these changes
Jul 17, 2024
|
Just need DCO |
Ticket [RHELAI-442](https://issues.redhat.com/browse/RHELAI-442) # Background RHEL AI ships with a script in `/usr/local/bin` called `ilab` which makes running `ilab` commands feel native even though they're actually running in a podman container # Issues * The script is outdated / used several different container images for different purposes, while it should be just using the single instructlab image * The volume mounts were incorrect, as instructlab now uses XDG paths * Unnecessary directory creation for `HF_CACHE` * Unnecessary GPU count logic * Script has unnecessary fiddling of `ilab` parameters, essentially creating a UX that deviates from the natural `ilab` CLI # Solutions * Changed script to use the single container image `IMAGE_NAME` (this was already the case mostly, except for old references to `LVLM_NAME` and `TRAIN_NAME` which no longer get replaced leading to a broken `PODMAN_COMMAND_SERVE`. Also adjusted entrypoint to use the `ilab` executable in the pyenv * Will now mount the host's `~/.config` and `~/.local` into the container's corresponding directories, for `instructlab` to use and for its config / data to persist across invocations * Will now mount `~/.cache` into the container's corresponding `.cache` directory, so that the information stored in the default `HF_CACHE` is also persisted across invocations * Removed unnecessary GPU count logic * Removed all parameter parsing / fiddling # Other changes Added secret/fake "shell" `ilab` subcommand which opens a shell in the wrapper's container, useful for troubleshooting issues with the wrapper itself Signed-off-by: Omer Tuchfeld <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ilabwrapper script adjustmentsTicket RHELAI-442
Background
RHEL AI ships with a script in
/usr/local/bincalledilabwhich makes runningilabcommands feel native even though they're actually running in a podman containerIssues
The script is outdated / used several different container images for different purposes, while it should be just using the single instructlab image
The volume mounts were incorrect, as instructlab now uses XDG paths
Unnecessary directory creation for
HF_CACHEUnnecessary GPU count logic
Script has unnecessary fiddling of
ilabparameters, essentially creating a UX that deviates from the naturalilabCLISolutions
Changed script to use the single container image
IMAGE_NAME(this was already the case mostly, except for old references toLVLM_NAMEandTRAIN_NAMEwhich no longer get replaced leading to a brokenPODMAN_COMMAND_SERVE. Also adjusted entrypoint to use theilabexecutable in the pyenvWill now mount the host's
~/.configand~/.localinto the container's corresponding directories, forinstructlabto use and for its config / data to persist across invocationsWill now mount
~/.cacheinto the container's corresponding.cachedirectory, so that the information stored in the defaultHF_CACHEis also persisted across invocationsRemoved unnecessary GPU count logic
Removed all parameter parsing / fiddling
Other changes
Added secret/fake "shell"
ilabsubcommand which opens a shell in the wrapper's container, useful for troubleshooting issues with the wrapper itself