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

Add LocalFileManagerDriver #738

Merged
merged 8 commits into from
Apr 11, 2024
Merged

Add LocalFileManagerDriver #738

merged 8 commits into from
Apr 11, 2024

Conversation

dylanholmes
Copy link
Contributor

@dylanholmes dylanholmes commented Apr 9, 2024

Changes:

  • Add BaseFileManagerDriver
  • Add LocalFileManagerDriver
  • Remove all existing fields from FileManager.
  • Add FileManager.file_manager_driver
  • Added optional BaseLoader.encoding field. This allows the LocalFileManagerDriver to save files with the same encoding as is used for loading them. This is possible without modifying BaseLoader, but not doing so would be more fragile and awkward.
  • Add BlobLoader

@dylanholmes dylanholmes force-pushed the feature/file-manager-driver branch 5 times, most recently from a4f6f0f to 77fef0e Compare April 9, 2024 21:12
@dylanholmes dylanholmes marked this pull request as ready for review April 9, 2024 21:18
@dylanholmes dylanholmes marked this pull request as draft April 10, 2024 13:12
@dylanholmes
Copy link
Contributor Author

Reverted to draft to make some changes I realized I needed+wanted to make while testing:

  • LocalFileManagerDriver. load_file needs to read in the file after the changes rebased that removed filepaths from the source of the loaders.
  • I want to move the usage of the loaders into the BaseFileManager, then have the subclasses implement a method for that reads/writes bytes only.

from typing import Optional
from attr import Factory, define, field
from griptape.artifacts import BaseArtifact, ErrorArtifact, TextArtifact, InfoArtifact, ListArtifact
import griptape.loaders as loaders
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This import alias is used to resolve a circular dependency between the drivers and loaders illustrated by the error shown below. Would be nice to have a more elegant solution, but I haven't found one yet.

_________________________________________________________ ERROR collecting tests/unit/loaders/test_blob_loader.py __________________________________________________________
ImportError while importing test module '/Users/dylan/Documents/griptape/tests/unit/loaders/test_blob_loader.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/unit/loaders/test_blob_loader.py:2: in <module>
    from griptape.loaders.blob_loader import BlobLoader
griptape/loaders/__init__.py:2: in <module>
    from .base_text_loader import BaseTextLoader
griptape/loaders/base_text_loader.py:11: in <module>
    from griptape.drivers import BaseEmbeddingDriver
griptape/drivers/__init__.py:96: in <module>
    from .file_manager.base_file_manager_driver import BaseFileManagerDriver
griptape/drivers/file_manager/base_file_manager_driver.py:6: in <module>
    from griptape.loaders import BaseLoader, BlobLoader, CsvLoader, ImageLoader, PdfLoader, TextLoader
E   ImportError: cannot import name 'BlobLoader' from partially initialized module 'griptape.loaders' (most likely due to a circular import) (/Users/dylan/Documents/griptape/griptape/loaders/__init__.py)

Copy link
Member

Choose a reason for hiding this comment

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

The griptape.drivers module is becoming quite large and causing more and more circular dependency issues as we integrate Drivers in different parts of the framework. I guess if this works it works.

@dylanholmes dylanholmes marked this pull request as ready for review April 10, 2024 20:34
Copy link
Member

@vasinov vasinov left a comment

Choose a reason for hiding this comment

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

Looks good!

@dylanholmes dylanholmes merged commit 3cac561 into dev Apr 11, 2024
6 checks passed
@dylanholmes dylanholmes deleted the feature/file-manager-driver branch April 11, 2024 08:32
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.

None yet

3 participants