search

patched-codes / patchwork

Automate development gruntwork like code reviews, patching and documentation with LLM workflows.
https://patched.codes
GNU Affero General Public License v3.0
813 stars 47 forks source link

Count tokens per LLM call #694

Closed CTY-git closed 2 weeks ago

patched-admin commented 2 weeks ago
The pull request review indicates that the modifications aim to add functionality to count tokens per LLM call by introducing 'request_tokens' and 'response_tokens' in various method call parameters and TypedDicts. However, potential issues were highlighted, such as type hinting errors, discrepancies in method names, and the need for additional documentation to clarify the purpose of these new fields. It is essential to ensure that necessary values are present before accessing them to prevent runtime errors and that the code is robust enough to handle scenarios where certain keys might not exist. Additionally, validation checks and proper handling of potential edge cases should be implemented to guarantee the accuracy and consistency of token counting throughout the codebase. ------
* File changed: [patchwork/steps/CallLLM/CallLLM.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-06684f9c0b7bb0e746ea0a10fd6f7cbe2e671ff44a4069e16cdf4faae231c409) The modifications seem to be adding the functionality to count tokens per LLM call. However, there are a few issues to note: 1. In the `__call` method, the type hint for the prompts parameter should be `list[dict]` instead of `list[list[dict]]`. This could potentially lead to type errors. 2. The new `_InnerCallLLMResponse` data class seems fine but could benefit from type hinting for the list attributes. 3. It's good to see the tokens being collected in lists, but there is a small discrepancy in the method names where the response token should be `completion.usage.completion_tokens` instead of `completion.usage.completion_tokens`. It might be a typo. 4. The completion tokens should be accessed as a property of the completion object to ensure that the correct count is obtained. Double-check this part to avoid introducing bugs related to token counting.
* File changed: [patchwork/steps/CallLLM/typed.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-7eee5f695e6a689150f65f01187311311ea8560450c433ec8637b6bffbf3795c) The code modification looks good in terms of adding new fields request_tokens and response_tokens to the CallLLMOutputs class. However, it would be better to have a comment explaining the purpose of these fields for better documentation and understanding.
* File changed: [patchwork/steps/LLM/LLM.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-62bc493f205830a467a8debcf3544ed237d317db71d88cc288e4f70e23f07af8) The code modifications in the pull request seem to be adding 'request_tokens' and 'response_tokens' to the parameters passed to a function. It looks like the changes are related to counting tokens per LLM call. However, there might be a potential bug in the new code if call_llm_outputs does not contain 'request_tokens' or 'response_tokens', which could lead to runtime errors. It's important to ensure that the necessary values are present in call_llm_outputs before accessing them in the function.
* File changed: [patchwork/steps/LLM/typed.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-e2b864381f414aeeb9fbd5a90666b56a6715b36c5bef760a0cbbebe94e0487c0) The addition of 'request_tokens' and 'response_tokens' to the LLMOutputs TypedDict seems appropriate for counting the tokens per LLM call. However, it may be beneficial to include comments or documentation specifying the purpose of these fields to ensure clarity for future maintainers.
* File changed: [patchwork/steps/SimplifiedLLM/SimplifiedLLM.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-f73ff142d98861fb66a6eebbfcfc772506c4ea9fe26336704a4135bd7c57e2b3) The addition of 'request_tokens' and 'response_tokens' seems to be missing context or documentation. It is unclear how these tokens are being used or why they are needed. It would be helpful to include comments or additional information to explain their purpose in the code. This lack of clarity could make it harder for others to maintain or debug the code in the future.
* File changed: [patchwork/steps/SimplifiedLLM/typed.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-ca05591855b1036af9bdaed433da4d2dace8917d6716106564b70d0679635f7a) The addition of 'request_tokens' and 'response_tokens' in 'SimplifiedLLMOutputs' seems to be in line with the title of the pull request 'Count tokens per LLM call'. However, it would be beneficial to have more context or documentation on how these token counts are being used or calculated to ensure correctness and efficiency. Additionally, it might be a good idea to add validation checks to prevent potential issues with counting tokens, such as negative values or incorrect counting methods.
* File changed: [patchwork/steps/SimplifiedLLMOnce/SimplifiedLLMOnce.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-afe4ee83eab4a19914ecfb9495e7627ef4c98a8a07d19af6e22a8fbdf9cc8fa0) The code modifications seem to be adding 'request_tokens' and 'response_tokens' to the method call parameters without any context. It's recommended to check if this change aligns with the overall design and purpose of the method. Additionally, it's crucial to ensure that these new fields are handled correctly to prevent potential bugs or unexpected behavior.
* File changed: [patchwork/steps/SimplifiedLLMOnce/typed.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-04ed4a18407cddb5e1a3f3315c451f01bec00479013f372eb2c81f3bc5b63ca3) The addition of 'request_tokens' and 'response_tokens' in the 'SimplifiedLLMOnceOutputs' TypedDict seems appropriate to track the number of tokens per LLM call. However, it would be beneficial to ensure that the assignment of these token counts is correctly implemented throughout the codebase to avoid discrepancies and potential bugs. It is advisable to review all relevant code changes to guarantee that these token counts are consistently and accurately populated.
* File changed: [patchwork/steps/SimplifiedLLMOncePB/SimplifiedLLMOncePB.py](https://github.com/patched-codes/patchwork/pull/694/files#diff-f9e84fdd8dc3a5f5c47cf44a6330e11a12c50fa420c6c95d6dd8d85843337077) The code modifications look good and seem to be in line with the original coding standards in the pull request. However, it is recommended to handle potential cases where 'llm_output' might be None or its keys might not exist, which could lead to KeyError. Adding proper checks to handle such scenarios will make the code more robust.
* File changed: [pyproject.toml](https://github.com/patched-codes/patchwork/pull/694/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711) Version in pyproject.toml has been updated from 0.0.49 to 0.0.50, no bugs or security vulnerabilities introduced. This change does not affect original coding standards in the pull request.