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

feat: add system info to the about_user prompt #171

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brayo-pip
Copy link
Collaborator

@brayo-pip brayo-pip commented Oct 8, 2024

Important

Adds system information to about_user prompt and introduces functions for system info retrieval in gptme/prompts.py.

  • Behavior:
    • Adds system information to about_user prompt in prompt_user() in gptme/prompts.py.
    • Introduces prompt_systeminfo() to generate system information prompt.
    • Implements get_system_distro() to retrieve Linux distribution name.
  • Dependencies:
    • Adds distro package to pyproject.toml for Linux distribution information.

This description was created by Ellipsis for 9b4832f. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to e610554 in 24 seconds

More details
  • Looked at 56 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_r7F2rP7I7n2tl2kZ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/prompts.py Outdated
version = os.uname().version
elif os.name == "nt":
distro = "Windows"
version = os.uname().version
Copy link

Choose a reason for hiding this comment

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

os.uname() is not available on Windows. Consider using platform.version() or platform.release() for Windows version information.

@brayo-pip
Copy link
Collaborator Author

@ellipsis-dev do another review

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 2b1f6d6 in 28 seconds

More details
  • Looked at 60 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_UV74QwS9w152cgUL


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/prompts.py Outdated
@@ -208,6 +210,21 @@ def prompt_tools(examples: bool = True) -> Generator[Message, None, None]:
prompt += "\n\n*End of Tools List.*"
yield Message("system", prompt.strip() + "\n\n")

def get_system_info() -> str:
Copy link

Choose a reason for hiding this comment

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

Consider adding exception handling in get_system_info() to manage potential errors when retrieving system information, such as when the distro package fails.

pyproject.toml Outdated
@@ -28,6 +28,7 @@ tomlkit = "*"
typing-extensions = "*"
platformdirs = "^4.3"
lxml = "*"
distro = "*"
Copy link
Owner

@ErikBjare ErikBjare Oct 8, 2024

Choose a reason for hiding this comment

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

I don't think such simple checks justifies a dependency.

Just use platform, you can call out to lsb_release -d (if it exists on system) if you want specific distro.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ErikBjare My system does not have lsb_release -d installed on default expecting more devices probably don't have installed by default. A workaround suggested is to read the contents of /etc/os-release

gptme/prompts.py Outdated
@@ -143,7 +145,7 @@ def prompt_user() -> Generator[Message, None, None]:
"""
config_prompt = get_config().prompt
about_user = config_prompt.get(
"about_user", "You are interacting with a human programmer."
"about_user", f"You are interacting with a human programmer. I am running a {get_system_info()} system."
Copy link
Owner

@ErikBjare ErikBjare Oct 8, 2024

Choose a reason for hiding this comment

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

I don't think we want to add it to the user prompt like this.

I'd like the config to be platform-portable (I copy mine between machines), and this wouldn't work for users with an old config.

prompt_user is also not included in non-interactive mode, but it's useful in non-interactive mode too.

Maybe add a prompt_systeminfo?

@brayo-pip brayo-pip force-pushed the master branch 2 times, most recently from 397c091 to 8fcd414 Compare October 8, 2024 15:03
@brayo-pip
Copy link
Collaborator Author

@ellipsis-dev review

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 8fcd414 in 39 seconds

More details
  • Looked at 62 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_bdClyRn5lcCwgDO3


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/prompts.py Outdated
@@ -208,8 +211,39 @@ def prompt_tools(examples: bool = True) -> Generator[Message, None, None]:
prompt += "\n\n*End of Tools List.*"
yield Message("system", prompt.strip() + "\n\n")

def prompt_systeminfo() -> Generator[Message, None, None]:
"""Generate the system information prompt."""
if os.name == "posix":
Copy link

Choose a reason for hiding this comment

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

The check for os.name == "darwin" is incorrect. os.name returns 'posix' for both Linux and macOS. Use platform.system() to differentiate between Linux and macOS.


yield Message( "system", prompt,)

def get_system_distro() -> str:
Copy link

Choose a reason for hiding this comment

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

Consider using the distro package to get the system distribution name for better accuracy and maintainability.

@brayo-pip
Copy link
Collaborator Author

@ellipsis-dev review

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 9b4832f in 22 seconds

More details
  • Looked at 62 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. gptme/prompts.py:231
  • Draft comment:
    Ensure consistent formatting for Message instantiation. Suggestion:
    yield Message("system", prompt)
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The prompt_systeminfo function has inconsistent formatting for the Message instantiation.

Workflow ID: wflow_lo0xDiy22ebBOste


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/prompts.py Outdated

def get_system_distro() -> str:
"""Get the system distribution name."""
regex = r"^NAME=\"([^\"]+)\""
Copy link

Choose a reason for hiding this comment

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

The regex pattern in get_system_distro is too specific. Consider using r'^NAME=("?)([^\n]+)\1$' to match entries with or without quotes.

@ErikBjare
Copy link
Owner

ErikBjare commented Oct 10, 2024

I just noticed/remembered I added this little line in the shell tool instructions to make it aware of macOS systems:

{'The platform is macOS.' if is_macos else ''}

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.

2 participants