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

fix(load): update error message in case of dataset not found locally and missing api keys #1589

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ArslanSaleem
Copy link
Collaborator

@ArslanSaleem ArslanSaleem commented Feb 4, 2025

Important

Update error message and add print statement in load() for missing local dataset and API key, with new test added.

  • Behavior:
    • Update error message in load() in pandasai/__init__.py when dataset not found locally and no API key is provided.
    • Add print statement in load() to indicate if dataset was loaded locally or fetched from remote server.
  • Tests:
    • Add test_load_missing_not_found_locally_and_no_remote_key in test_pandasai_init.py to verify updated error message.

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

@ArslanSaleem ArslanSaleem requested a review from gventuri February 4, 2025 16:54
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.56%. Comparing base (18e4f85) to head (d6446b6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1589      +/-   ##
==========================================
+ Coverage   89.54%   89.56%   +0.01%     
==========================================
  Files          67       67              
  Lines        2488     2492       +4     
==========================================
+ Hits         2228     2232       +4     
  Misses        260      260              
Flag Coverage Δ
unittests 89.56% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@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 d6446b6 in 3 minutes and 2 seconds

More details
  • Looked at 97 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pandasai/__init__.py:222
  • Draft comment:
    Consider rewording the error message. For example: "The dataset '{dataset_path}' was not found in your local datasets directory, and no API key was provided. Please set a valid API key to fetch the dataset from the remote server." This improves clarity and conciseness.
  • Reason this comment was not posted:
    Marked as duplicate.
2. pandasai/__init__.py:243
  • Draft comment:
    Avoid using print() in library code. Consider using a proper logging mechanism instead.
  • Reason this comment was not posted:
    Marked as duplicate.
3. tests/unit_tests/test_pandasai_init.py:143
  • Draft comment:
    Ensure that tests are updated to reflect the changes in exception messages and types. The new detailed message should be verified in tests consistently.
  • Reason this comment was not posted:
    Marked as duplicate.
4. tests/unit_tests/test_pandasai_init.py:160
  • Draft comment:
    Test 'test_load_missing_api_url' now expects DatasetNotFound. Ensure this test clearly distinguishes the scenario of a missing remote dataset (HTTP 404) versus missing credentials, as these branches raise different exceptions.
  • Reason this comment was not posted:
    Comment did not seem useful.
5. tests/unit_tests/test_pandasai_init.py:220
  • Draft comment:
    Update the expected error message in 'test_load_without_api_credentials' to match the new, more descriptive error message. Also, consider phrasing improvements like using 'permissions' instead of 'permits'.
  • 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_IPwce7Jr2L4rtkVa


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.

pandasai/__init__.py Show resolved Hide resolved
if local_dataset_exists
else "Dataset fetched successfully from the remote server."
)
print(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using print for success messages. It’s better to use a logging mechanism to allow configurable verbosity in production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ellipsis-dev, can you suggest the fix here?

Copy link
Contributor

Choose a reason for hiding this comment

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

To address the review comments:

  1. Error Message Update: Modify the error message to mention both the missing API key and API URL, as the condition checks for both.

  2. Logging: Replace the print statement with a logging mechanism. You can use Python's built-in logging module to allow configurable verbosity. For example:

    import logging
    logger = logging.getLogger(__name__)
    
    message = (
        "Dataset loaded successfully."
        if local_dataset_exists
        else "Dataset fetched successfully from the remote server."
    )
    logger.info(message)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ArslanSaleem I suggest we just get a rid of the print here

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