-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(docs): sandbox documentation and example corrected #1682
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
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.
👍 Looks good to me! Reviewed everything up to ea45ef6 in 51 seconds
More details
- Looked at
180lines of code in4files - Skipped
1files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. docs/v3/agent.mdx:243
- Draft comment:
The new 'Using Sandbox' section is clear. Consider adding a note about cleaning up resources (stopping the sandbox) in case of errors for best practices. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. docs/v3/large-language-models.mdx:80
- Draft comment:
The header change to 'How to set up any LLM?' improves clarity, but ensure that the description below still clearly communicates that LiteLLM is used for a unified interface. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. docs/v3/privacy-security.mdx:39
- Draft comment:
The updated sandbox code snippet now includes setting the API key, which is a good addition. The example clearly demonstrates reading CSV data and showing output. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. pandasai/exceptions.py:211
- Draft comment:
The error message update now correctly references 'PandaAI.api_key.set()'. This provides better guidance to users. Consider ensuring consistent formatting across all exception messages. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. docs/v3/agent.mdx:243
- Draft comment:
Sandbox documentation update is clear and helpful. Consider emphasizing explicitly that Docker must be installed and running (even though it's mentioned) to avoid any ambiguity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. docs/v3/large-language-models.mdx:80
- Draft comment:
Renaming the header from 'LiteLLM' to 'How to set up any LLM?' improves clarity. Ensure that this updated naming is consistently used throughout the documentation. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. docs/v3/privacy-security.mdx:39
- Draft comment:
The updated sandbox usage example now uses pai.chat with a DataFrame and sandbox, aligning with the latest API usage. This is a good improvement for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. pandasai/exceptions.py:211
- Draft comment:
The default error message in PandaAIApiKeyError has been updated to use PandaAI.api_key.set(), matching current API usage. Ensure related documentation and tests reflect this change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. docs/v3/agent.mdx:197
- Draft comment:
Typo: In the sentence 'First of all, you'll need to generated an API key', 'generated' should be corrected to 'generate'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. docs/v3/agent.mdx:205
- Draft comment:
Typo: The class name 'BambooVectorStor' appears to be missing an 'e'. It should likely be 'BambooVectorStore'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. docs/v3/large-language-models.mdx:168
- Draft comment:
Typo: 'should me' should be corrected to 'should be' to maintain clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. pandasai/exceptions.py:132
- Draft comment:
Typographical suggestion: Consider renaming the class 'UnSupportedLogicUnit' to 'UnsupportedLogicUnit' for consistency in capitalization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pandasai/exceptions.py:142
- Draft comment:
Typographical correction: In the docstring for 'InvalidWorkspacePathError', change 'environment variable of workspace exist but path is invalid' to 'environment variable for workspace exists but the path is invalid'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pandasai/exceptions.py:175
- Draft comment:
Typographical suggestion: In the 'InvalidOutputValueMismatch' docstring, consider revising 'doesn't match with type' to 'doesn't match the type' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ZnB2s87MddFfkswX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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 to me! Incremental review on ea53254 in 1 minute and 32 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. tests/unit_tests/helpers/test_session.py:19
- Draft comment:
Updated error message: The setter now uses PandaAI.api_key.set() instead of PandaAI.set_api_key(). Ensure consistency with documentation and all related parts of the codebase. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment suggests ensuring consistency with documentation and related parts of the codebase, which is not allowed. However, it does point out a change in method usage, which is relevant. The comment could be improved by focusing on the change itself and suggesting to update documentation if necessary.
2. tests/unit_tests/helpers/test_session.py:29
- Draft comment:
Updated error message here as well to use PandaAI.api_key.set(). Keep messages consistent across tests. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. tests/unit_tests/helpers/test_session.py:80
- Draft comment:
Error message update in get_pandaai_session test reflects new instructions. Confirm use across all tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm the use of an error message update across all tests. This violates the rule against asking the author to confirm or double-check things. Therefore, this comment should not be approved.
4. tests/unit_tests/helpers/test_session.py:19
- Draft comment:
Updated error message text now correctly references 'PandaAI.api_key.set()' instead of 'PandaAI.set_api_key()'. Consider centralizing this constant to avoid duplication. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a test file that's specifically testing error messages. The repetition is intentional because each test is verifying the exact same error message in different scenarios. Centralizing the message could actually make the tests less clear and direct. In test files, some duplication is acceptable when it makes the tests more explicit and easier to understand.
The comment has a valid point about DRY principles. If the error message changes, we'd need to update it in multiple places.
However, in test files, explicit assertions are more valuable than DRY principles. The duplication here serves a purpose - it makes each test self-contained and clear about what it's testing.
The comment should be deleted. While it suggests a valid coding practice, in the context of test files this kind of duplication is acceptable and even beneficial for test clarity and maintainability.
5. tests/unit_tests/helpers/test_session.py:30
- Draft comment:
Updated error message now correctly references 'PandaAI.api_key.set()' instead of 'PandaAI.set_api_key()'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it only states that an error message has been updated to reference a different method. It does not provide any actionable feedback or suggestions for improvement.
6. tests/unit_tests/helpers/test_session.py:80
- Draft comment:
Error message updated to reference 'PandaAI.api_key.set()'. Ensure this change is reflected in both documentation and other tests to keep consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is about a change in the diff, specifically the error message update. However, the comment is asking to "ensure" something is done in documentation and other tests, which violates the rule about not making speculative comments or asking for verification. The actual change in the error message text is already being made consistently across all test cases in this file.
The comment might be pointing out a legitimate concern about documentation consistency. Documentation updates are important when API methods change.
While documentation consistency is important, the comment violates the rules by asking for verification and making speculative suggestions about other files. We should only comment on the current file's changes.
The comment should be deleted as it violates the rules against asking for verification and making speculative comments about other files.
7. tests/unit_tests/helpers/test_session.py:56
- Draft comment:
Typographical error: the environment variable keys are inconsistently named. In patch.dict decorators (e.g., line 56), the key is 'PANDABI_API_KEY' (and similarly 'PANDABI_API_URL'), but the error messages and documentation refer to 'PANDASAI_API_KEY'. Please ensure the naming is consistent across the code and tests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_rxyIpMQwrpKCypwz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1682 +/- ##
=======================================
Coverage 91.88% 91.88%
=======================================
Files 72 72
Lines 2796 2796
=======================================
Hits 2569 2569
Misses 227 227
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* commit '140098cf5b6de288102bce8f50096a63e3f8951b': fix(read_csv): lower file name (sinaptik-ai#1681) fix(docs): sandbox documentation and example corrected (sinaptik-ai#1682)
Important
This PR updates sandbox usage documentation and corrects API key setting instructions in code and tests.
agent.mdx,large-language-models.mdx, andprivacy-security.mdx.exceptions.pyandtest_session.py.PandaAIApiKeyErrormessage inexceptions.pyto usePandaAI.api_key.set().test_session.pyto reflect the updated API key setting method.This description was created by
for ea53254. It will automatically update as commits are pushed.