Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container improvements #540

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Robinsane
Copy link

  1. added 3 env variables to make life easier configuring shell_gpt containers:
    IN_CONTAINER: setting this one to true, helps fix some default things that aren't handy when in a container:
  • The default for interaction becomes False, since [E]xecute on host doesn't work when you get the output from a container
  • Shell_gpt will not check the machine for os and shell (which would be fetched from the container), but will check what the user has configured in following env vars: OS_OUTSIDE_CONTAINER and SHELL_OUTSIDE_CONTAINER
  1. adjusted Dockerfile to use new env vars + added an example ollama Dockerfile

Copy link
Owner

@TheR1D TheR1D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also update Config and Docker section in README.md please?

sgpt/config.py Outdated Show resolved Hide resolved
sgpt/app.py Outdated Show resolved Hide resolved
Dockerfile_ollama Outdated Show resolved Hide resolved
Dockerfile Outdated
@@ -1,5 +1,9 @@
FROM python:3-slim

ENV IN_CONTAINER=true
ENV OS_OUTSIDE_CONTAINER="Linux/Red Hat Enterprise Linux 8.8 (Ootpa)"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an argument, so user can setup the image with specific OS and SHELL names.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the upcoming commit I'll leave it as ENV variable that is in the config.py DEFAULT_CONFIG.
Reason being that it might be handy to leave it interchangeable, e.g.:

  • All of a sudden you need to work on a server with an os / shell you're very unfamiliar with. You can temporarily overwrite your os and shell name of your often used shellgpt to mock this environment, without having to install on that server / install in a similar virtual local environment
  • Similar to above, if I'd like to use shellgpt containers for multiple setups: RHEL, Debian, Ubuntu, powershell, MacOs, ... I can run the same image each time, as long as I overwrite the OS and shell to mock these envs. If I had to make a different image each time that'd be a waste of diskspace and image build time in my opinion.

sgpt/role.py Outdated Show resolved Hide resolved
@ismaelchris
Copy link

These settings worked for me:

IN_CONTAINER=false
USE_LITELLM=true

@Robinsane Robinsane requested a review from TheR1D April 15, 2024 13:56
@@ -57,7 +57,7 @@ def main(
rich_help_panel="Assistance Options",
),
interaction: bool = typer.Option(
True,
True if cfg.get("SHELL_INTERACTION") != "false" else False,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
True if cfg.get("SHELL_INTERACTION") != "false" else False,
cfg.get("SHELL_INTERACTION") == "true",

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote it this way to ensure the normal default behaviour is set, also when SHELL_INTERACTION contains an unintended / weird value

sgpt/role.py Outdated Show resolved Hide resolved
sgpt/role.py Outdated Show resolved Hide resolved
sgpt/config.py Outdated Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 3 to 6
ENV SHELL_INTERACTION=false
ENV OVERWRITE_OS_NAME=""
ENV OVERWRITE_SHELL_NAME=""

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ENV SHELL_INTERACTION=false
ENV OVERWRITE_OS_NAME=""
ENV OVERWRITE_SHELL_NAME=""
ARG OVERWRITE_OS_NAME=default
ARG OVERWRITE_SHELL_NAME=default
ENV SHELL_INTERACTION=false
ENV OVERWRITE_OS_NAME=${OVERWRITE_OS_NAME}
ENV OVERWRITE_SHELL_NAME=${OVERWRITE_SHELL_NAME}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you misunderstand Docker's ARG:
These are env variables that only exist during the building process of the container image.
Once the image exists, these are gone forever.
Please refer the dockerfile arg documentation for a better, more complete explanation.

ENV sets environment variables that remain, and can be used set as arguments with --env as desired when starting the container

README.md Outdated Show resolved Hide resolved
Robinsane and others added 5 commits April 24, 2024 09:57
default OVERWRITE_OS_NAME value "default" + got rid of redundant else

Co-authored-by: Farkhod Sadykov <[email protected]>
OVERWRITE_SHELL_NAME default value "default" + got rid of redundant else

Co-authored-by: Farkhod Sadykov <[email protected]>
@Robinsane Robinsane requested a review from TheR1D April 24, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants