feat: implement gptme-util CLI for utilities #261
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
- Added tools list and info commands - Added language tag support - Reused existing code from commands.py - Added initial tests for utility CLI - Moved util.py to util/__init__.py for better organization
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 3a2573d in 48 seconds
More details
- Looked at
246lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. gptme.toml:1
- Draft comment:
The addition ofgptme/chat.pyto thefileslist seems unrelated to the PR's purpose. Ensure this change is intentional and necessary. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new filegptme/chat.pyto thefileslist ingptme.toml. This change is not explained in the PR description or related to the issue being fixed. It might be an oversight or unrelated change.
2. gptme/util/__init__.py:24
- Draft comment:
The import statement should be corrected to import fromgptme.clipboardinstead of..clipboard. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change in the import statement from '.clipboard' to '..clipboard' is due to the file being moved from 'gptme/util.py' to 'gptme/util/init.py'. The relative import '..clipboard' is correct because it refers to the clipboard module in the parent directory of the current package. The comment suggesting an absolute import is not necessary unless there is a specific reason to prefer absolute imports, which is not mentioned in the comment.
I might be missing some context about the project's import style guidelines, which could prefer absolute imports. However, without explicit evidence or guidelines, the relative import seems correct given the file structure change.
The comment does not provide any evidence or reasoning for preferring an absolute import, and the relative import appears correct given the file's new location.
The comment should be deleted because the relative import '..clipboard' is correct given the file's new location, and there is no strong evidence to support the suggested change to an absolute import.
3. gptme/util/cli.py:28
- Draft comment:
Consider handling the case whereget_user_conversations()returns an empty list to improve user experience. - Reason this comment was not posted:
Confidence changes required:50%
Thechats_listfunction ingptme/util/cli.pydoes not handle the case whereget_user_conversations()returns an empty list. This could lead to a confusing user experience if no chats are available.
4. gptme/util/cli.py:81
- Draft comment:
Consider adding validation for themodelparameter to ensure it is valid before callingtiktoken.encoding_for_model(model). - Reason this comment was not posted:
Confidence changes required:50%
Thetokens_countfunction ingptme/util/cli.pyusestiktoken.encoding_for_model(model)without checking if the model is valid. This could lead to runtime errors if an invalid model is provided.
5. gptme/util/cli.py:135
- Draft comment:
Consider simplifying the multi-line f-string for printing tool information to improve readability. - Reason this comment was not posted:
Confidence changes required:30%
Thetools_listfunction ingptme/util/cli.pyuses a multi-line f-string for printing tool information. This can be simplified to improve readability.
6. tests/test_util_cli.py:20
- Draft comment:
Consider adding assertions to verify the output of thechats_listcommand to ensure it behaves as expected. - Reason this comment was not posted:
Confidence changes required:50%
The test forchats_listintests/test_util_cli.pydoes not verify the output, which could lead to false positives if the command does not behave as expected.
Workflow ID: wflow_vUFfDJZhLEPkTArB
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/util/cli.py
Outdated
|
|
||
| @chats.command("ls") | ||
| @click.option("-n", "--limit", default=20, help="Maximum number of chats to show.") | ||
| def chats_list(limit: int): |
There was a problem hiding this comment.
The chats_list function duplicates the functionality of list_chats in gptme/tools/chats.py. Consider using or extending list_chats instead.
- function
list_chats(chats.py)
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #261 +/- ##
==========================================
+ Coverage 70.66% 72.45% +1.78%
==========================================
Files 61 62 +1
Lines 3958 4055 +97
==========================================
+ Hits 2797 2938 +141
+ Misses 1161 1117 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ErikBjare
commented
Nov 17, 2024
gptme.toml
Outdated
| @@ -1,2 +1,2 @@ | |||
| files = ["README.md", "Makefile"] | |||
| files = ["README.md", "Makefile", "gptme/chat.py"] | |||
- Added empty list handling in chats_list - Added model validation in tokens_count - Added comprehensive test cases - Skip context test until module is available
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 2af94f8 in 26 seconds
More details
- Looked at
150lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gptme/util/cli.py:82
- Draft comment:
Thetokens_countfunction should return a non-zero exit code when no text is provided. This is a common practice for CLI tools to signal errors. - Reason this comment was not posted:
Marked as duplicate.
2. tests/test_util_cli.py:82
- Draft comment:
Thetokens_countfunction should return a non-zero exit code when no text is provided. This is a common practice for CLI tools to signal errors. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_hHX0Fk5mw1cXSd38
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.
- Import _ModelDictMeta from models.py instead of duplicating definition - Fixes code duplication identified by pylint
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d73de53 in 9 seconds
More details
- Looked at
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gptme/llm_openai_models.py:1
- Draft comment:
Ensure that the module.modelsexists and contains_ModelDictMeta. This change could lead to an import error if.modelsis not correctly set up. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_sDmcfNNKrgPp3eZ9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f9e4aee in 18 seconds
More details
- Looked at
75lines of code in3files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gptme/util/cli.py:28
- Draft comment:
The import statement forformat_conversationis unnecessary here since it's already imported inlist_conversations. Consider removing it. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'format_conversation' in 'cli.py' is unnecessary since it's already imported in 'list_conversations'.
2. gptme/util/cli.py:32
- Draft comment:
Consider usingclick.echoinstead ofprintfor consistent CLI output handling. This applies to otherprintstatements in this file as well. - Reason this comment was not posted:
Confidence changes required:50%
Theprintfunction is used incli.pyfor output, which is not ideal for CLI applications. It's better to useclick.echofor consistent output handling.
Workflow ID: wflow_JguIMLZhjgHNS8cG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
force-pushed
the
dev/gptme-util-cli
branch
from
f9e4aee
to
cd8ef84
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cd8ef84 in 18 seconds
More details
- Looked at
92lines of code in3files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. gptme/logmanager.py:12
- Draft comment:
The import ofCallablefromcollections.abcis unnecessary as it is already imported fromtyping. Consider removing it to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
The import of 'Callable' from 'collections.abc' is unnecessary as it is already imported from 'typing'.
2. gptme/logmanager.py:379
- Draft comment:
Thefoundboolean flag is unnecessary. You can check ifconversationsis empty instead of usingfound. This applies to the usage incli.pyas well. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new functionlist_conversationswhich is used incli.py. The function returns a tuple with a boolean flagfoundwhich is unnecessary since the list itself can be checked for emptiness.
3. gptme/util/cli.py:27
- Draft comment:
The logic for printing language tags intools_listcan be simplified. Instead of checking for specific line prefixes, consider using a more direct approach to extract and print the language tags. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new CLI commandtools listwith an option--langtags. The logic for printing language tags is a bit convoluted and can be simplified.
4. gptme/util/cli.py:29
- Draft comment:
Thefoundflag returned bylist_conversationsis unnecessary. You can check ifconversationsis empty instead of usingfound. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new CLI commandchats lswhich useslist_conversations. Thefoundflag is unnecessary and can be removed.
Workflow ID: wflow_zwsJ5pATPUagROLi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6cca401 in 10 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gptme/llm_openai_models.py:4
- Draft comment:
Remove the unnecessary comment# fmt: skipas it doesn't add value here. - Reason this comment was not posted:
Confidence changes required:33%
The use of TYPE_CHECKING and the import statement inside the if block is correct for avoiding circular imports during runtime. However, the comment about formatting is unnecessary and should be removed for cleaner code.
Workflow ID: wflow_3XOaanDeQ1QG1EB3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ede4361 in 16 seconds
More details
- Looked at
29lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gptme/util/__init__.py:326
- Draft comment:
Consider moving the import statements forlen_tokensandinit_toolsto the top of the file to improve readability and maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The imports are intentionally placed within the function, likely for a reason such as avoiding circular dependencies or reducing initial load time. The# fmt: skipand# noreordercomments suggest that the current placement is deliberate. The suggestion to move them for readability does not consider these factors and may not be appropriate.
I might be missing the context of why the imports are placed there, but the presence of# fmt: skipand# noreorderstrongly suggests intentional placement. The comment does not provide a compelling reason to change this.
The presence of# fmt: skipand# noreorderis strong evidence that the current placement is intentional and necessary. The comment does not account for this, making it less valid.
The comment should be deleted because the import placement is intentional, as indicated by# fmt: skipand# noreorder, and the suggestion does not consider this.
Workflow ID: wflow_iORmOwnk83yzDPmE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d92f291 in 8 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. docs/cli.rst:25
- Draft comment:
Add a title for thegptme-utilcommand section to maintain consistency and improve readability. - Reason this comment was not posted:
Confidence changes required:50%
The CLI reference section in the documentation is missing a title for the new gptme-util command. This could lead to confusion for users looking for information about the new CLI command.
Workflow ID: wflow_LLMCahDfF0gbuLJb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
commented
Nov 17, 2024
ErikBjare
changed the title
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 66ea9f8 in 25 seconds
More details
- Looked at
444lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gptme/util/cli.py:147
- Draft comment:
Simplify the multi-line string for printing tool information.
print(f"{status} {tool.name}\n {tool.desc}")
- Reason this comment was not posted:
Confidence changes required:50%
Thetools_listfunction uses a multi-line string for printing tool information, which is unnecessary and can be simplified.
2. gptme/util/cli.py:150
- Draft comment:
Simplify the multi-line string for printing tool information.
print(f"{status} {tool.name}\n {tool.desc}")
- Reason this comment was not posted:
Confidence changes required:50%
Thetools_listfunction uses a multi-line string for printing tool information, which is unnecessary and can be simplified.
Workflow ID: wflow_vH04JsMMhaKNuHPH
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.
|
|
||
| @context.command("generate") | ||
| @click.argument("path", type=click.Path(exists=True)) | ||
| def context_generate(_path: str): |
There was a problem hiding this comment.
context_generate is a placeholder and does not perform any operations. Implement the function or remove it if not needed.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d060108 in 12 seconds
More details
- Looked at
34lines of code in2files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pyproject.toml:62
- Draft comment:
The specific version constraint forpylintwas removed and replaced withpylint = "*". Ensure this is intentional, as it may lead to using the latest version, which could introduce breaking changes. - Reason this comment was not posted:
Confidence changes required:50%
The Makefile has a redundant line for pylint in the pyproject.toml file. The linepylint = "^3.3.1"was removed, but the new linepylint = "*"was added under[tool.poetry.group.dev.dependencies]. This change is correct as it moves pylint to the dev dependencies, but the removal of the specific version constraint should be noted.
Workflow ID: wflow_xWceDMrEGtpKXBJx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…l chats list --summarize`
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1aa576a in 21 seconds
More details
- Looked at
114lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. gptme/util/cli.py:40
- Draft comment:
Consider renamingmax_resultstolimitin thelist_chatsfunction to maintain consistency with thelist_conversationsfunction ingptme/logmanager.py. - Reason this comment was not posted:
Confidence changes required:50%
Thelist_chatsfunction ingptme/util/cli.pyusesmax_resultsas a parameter, but thelist_conversationsfunction ingptme/logmanager.pyuseslimit. This inconsistency can lead to confusion and potential bugs.
2. gptme/util/cli.py:35
- Draft comment:
Consider moving the model initialization outside thechats_listfunction to improve performance by avoiding repeated initialization. - Reason this comment was not posted:
Confidence changes required:50%
Thechats_listfunction ingptme/util/cli.pyinitializes the model for summarization inside the function, which is not ideal for performance. It should be initialized outside the function to avoid repeated initialization.
3. gptme/tools/chats.py:49
- Draft comment:
Ensure that themetadataparameter is explicitly passed to theconv.format()call to avoid unexpected behavior if the default value changes. - Reason this comment was not posted:
Confidence changes required:50%
Thelist_chatsfunction ingptme/tools/chats.pyusesmetadata=Truein theconv.format()call, but themetadataparameter is not passed to the function. This can lead to unexpected behavior if the default value ofmetadatachanges.
Workflow ID: wflow_5ysNG7JdMqQvsxFg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 4ad31b2 in 25 seconds
More details
- Looked at
128lines of code in3files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gptme/util/cli.py:170
- Draft comment:
Usingsys.exit(1)directly can make testing and integration more difficult. Consider returning an exit code from the function and letting the CLI framework handle it. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_DFSZx0H1GjBUID7b
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.
ErikBjare
force-pushed
the
dev/gptme-util-cli
branch
from
4ad31b2
to
7edb198
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7edb198 in 20 seconds
More details
- Looked at
128lines of code in3files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. gptme/util/cli.py:92
- Draft comment:
Usingsys.exit(1)is appropriate here to indicate an error state in a CLI application. Ensure this is consistent across similar functions. - Reason this comment was not posted:
Confidence changes required:10%
The use ofsys.exit(1)is appropriate for CLI applications to indicate an error, but it should be consistent across the codebase. The change fromreturntosys.exit(1)intokens_countandtools_infofunctions is correct for indicating an error state.
2. gptme/util/cli.py:170
- Draft comment:
Usingsys.exit(1)is appropriate here to indicate an error state in a CLI application. Ensure this is consistent across similar functions. - Reason this comment was not posted:
Confidence changes required:10%
The use ofsys.exit(1)is appropriate for CLI applications to indicate an error, but it should be consistent across the codebase. The change fromreturntosys.exit(1)intokens_countandtools_infofunctions is correct for indicating an error state.
3. tests/test_util_cli.py:32
- Draft comment:
Ensure the exit code is exactly 1 for an invalid tool to indicate an error state.
assert result.exit_code == 1 # returns 1 for not found tool
- Reason this comment was not posted:
Confidence changes required:50%
The test fortools infocommand correctly checks for both valid and invalid tool names. However, the test for invalid tool should ensure that the exit code is exactly 1, which is the standard for indicating an error.
Workflow ID: wflow_acPqoz1UC1FUyrck
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
Fixes #260
Built by Bob (#259)
Important
Introduces a new CLI for utility commands with language tag support and reorganizes utility code structure.
tools listandtools infocommands inutil/cli.py.tools_list().commands.pyfor tool-related functionalities.test_util_cli.py.util.pytoutil/__init__.pyfor better organization.This description was created by
for 7edb198. It will automatically update as commits are pushed.