-
-
Notifications
You must be signed in to change notification settings - Fork 193
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 platform info to the system prompt #171
Conversation
There was a problem hiding this 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 in2
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 |
There was a problem hiding this comment.
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.
@ellipsis-dev do another review |
There was a problem hiding this 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 in2
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: |
There was a problem hiding this comment.
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 = "*" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
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
?
397c091
to
8fcd414
Compare
@ellipsis-dev review |
There was a problem hiding this 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 in1
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": |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellipsis-dev compile this regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellipsis-dev review |
There was a problem hiding this 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 in1
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 forMessage
instantiation. Suggestion:
yield Message("system", prompt)
- Reason this comment was not posted:
Confidence changes required:50%
Theprompt_systeminfo
function has inconsistent formatting for theMessage
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=\"([^\"]+)\"" |
There was a problem hiding this comment.
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.
df07566
to
60f5360
Compare
I just noticed/remembered I added this little line in the shell tool instructions to make it aware of macOS systems: Line 53 in f258602
|
056a17b
to
428f4a6
Compare
…to the system prompt);
@ErikBjare Review this whenever 👍 |
Closes #166 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Just a minor nit on prompt formatting. Also recommend you set up pre-commit
so that the code gets formatted correctly (I should add a formatting check to CI).
Co-authored-by: Erik Bjäreholt <[email protected]>
Co-authored-by: Erik Bjäreholt <[email protected]>
Nice, thanks! |
Got this weird output when testing in #225: So we accidentally got the kernel version and not the distro version. |
def get_system_distro() -> str: | ||
"""Get the system distribution name.""" | ||
regex = re.compile(r"^NAME=\"?([^\"]+)\"?") | ||
if os.path.exists("/etc/os-release"): | ||
with open("/etc/os-release") as f: | ||
for line in f: | ||
matches = re.search(regex, line) | ||
if matches: | ||
return matches.string[matches.start(1) : matches.end(1)] | ||
return "Linux" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get the distro version here (BUILD_ID=rolling
on Arch, VERSION_ID="22.04"
on Ubuntu) instead of using platform.uname().release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check into this. Also getting the same on my laptop, capturing the kernel version instead of the distro version.
Important
Adds system information to
about_user
prompt and introduces functions for system info retrieval ingptme/prompts.py
.about_user
prompt inprompt_user()
ingptme/prompts.py
.prompt_systeminfo()
to generate system information prompt.get_system_distro()
to retrieve Linux distribution name.distro
package topyproject.toml
for Linux distribution information.This description was created by for 9b4832f. It will automatically update as commits are pushed.