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(FileManager): updated path in create and pull methods #1579

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

scaliseraoul
Copy link
Contributor

@scaliseraoul scaliseraoul commented Feb 3, 2025


Important

Introduce abs_path method in FileManager for absolute path handling and update relevant methods in pandasai to use it.

  • File Management:
    • Add abs_path() method to FileManager and implement in DefaultFileManager to return absolute file paths.
    • Use abs_path() in create() and pull() in pandasai/__init__.py for file path handling.
    • Use abs_path() in LocalDatasetLoader._read_csv_or_parquet() in local_loader.py.
    • Use abs_path() in DataFrame.push() and DataFrame.pull() in base.py.
  • Tests:
    • Update test_validate_llm_with_langchain() in test_config.py to check langchain_llm attribute.

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

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 9a69b8f in 54 seconds

More details
  • Looked at 139 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. pandasai/__init__.py:121
  • Draft comment:
    Consider renaming 'parquet_file_path_abs_path' to something like 'abs_parquet_file_path' for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While both naming conventions would work and be understandable, this is a very minor stylistic preference. The current name is already clear and descriptive. The suggestion doesn't materially improve code quality or fix any actual issues. This falls under the rule about not making comments that are obvious or unimportant.
    The suggested name might be marginally more consistent with common Python naming patterns where adjectives often come first. The current name might be slightly more verbose than necessary.
    The current name is still perfectly clear and follows a valid naming convention. This is too minor of a stylistic preference to warrant a comment.
    Delete this comment as it suggests a minor stylistic change that doesn't materially improve code quality or fix any actual issues.
2. pandasai/data_loader/local_loader.py:47
  • Draft comment:
    Good use of file_manager.abs_path for both parquet and CSV. Ensure tests cover file path resolution for local files.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
3. pandasai/dataframe/base.py:169
  • Draft comment:
    Updated file path composition in the push method is clear; using self.path directly improves consistency.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
4. tests/unit_tests/test_config.py:52
  • Draft comment:
    Updated assertion with 'langchain_llm' is clearer. Ensure that unit tests also cover scenarios where the langchain integration is absent.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
5. pandasai/__init__.py:121
  • Draft comment:
    Consider renaming 'parquet_file_path_abs_path' to 'absolute_parquet_path' for better clarity.
  • Reason this comment was not posted:
    Marked as duplicate.
6. pandasai/data_loader/local_loader.py:49
  • Draft comment:
    Good use of file_manager.abs_path for resolving file paths. Consider handling file-not-found exceptions later if needed.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
7. pandasai/dataframe/base.py:169
  • Draft comment:
    Changing the schema and data file paths to use 'self.path' (without 'datasets/') is fine if file_manager.abs_path appends the base path. Confirm that all consumers provide the expected relative path.
  • Reason this comment was not posted:
    Comment did not seem useful.
8. pandasai/dataframe/base.py:231
  • Draft comment:
    Reinitializing the DataFrame via init in pull() works but may risk losing custom state; ensure this behavior is intended.
  • 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.
9. pandasai/helpers/filemanager.py:41
  • Draft comment:
    Improve the abs_path() docstring to avoid using curly braces. For example, 'Returns the absolute path for the given file path.'
  • Reason this comment was not posted:
    Marked as duplicate.
10. tests/unit_tests/test_config.py:52
  • Draft comment:
    The update from '_llm' to 'langchain_llm' improves clarity. Ensure that the LangchainLLM interface is well-documented to reflect this property.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_CmuyodztHy03FqKh


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/helpers/filemanager.py Show resolved Hide resolved
@gventuri gventuri merged commit 98ea589 into sinaptik-ai:main Feb 3, 2025
12 checks passed
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.

4 participants