Adds the ability to pass files to ramalama run
#1570
Conversation
ieaves
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver,
slp and
engelmi
as code owners
Reviewer's GuideImplements file input support for Sequence diagram for CLI file upload flowsequenceDiagram
actor User
participant CLI as CLI
participant FileUpLoader
participant FileHandlers as File Handlers
participant Model
User->>CLI: run ramalama run --input <file/dir>
CLI->>FileUpLoader: Initialize with file/dir path
FileUpLoader->>FileHandlers: Load and concatenate file contents
FileHandlers-->>FileUpLoader: File contents
FileUpLoader-->>CLI: Concatenated file contents
CLI->>Model: Inject file contents into prompt and run model
Model-->>CLI: Model output
CLI-->>User: Display output
Sequence diagram for chat file upload flowsequenceDiagram
actor User
participant Chat as Chat Command
participant FileUpLoader
participant FileHandlers as File Handlers
participant Model
User->>Chat: Start chat with --input <file/dir>
Chat->>FileUpLoader: Initialize with file/dir path
FileUpLoader->>FileHandlers: Load and concatenate file contents
FileHandlers-->>FileUpLoader: File contents
FileUpLoader-->>Chat: Concatenated file contents
Chat->>Model: Inject file contents into chat prompt and run model
Model-->>Chat: Model output
Chat-->>User: Display output
Entity relationship diagram for supported file typeserDiagram
FILE_UPLOADER ||--o{ FILE : loads
FILE {
string file_path
string extension
}
FILE_UPLOADER {
string document_delimiter
}
TXT_FILE_UPLOAD ||--|| FILE : is_a
PDF_FILE_UPLOAD ||--|| FILE : is_a
FILE_UPLOADER ||--o{ TXT_FILE_UPLOAD : uses
FILE_UPLOADER ||--o{ PDF_FILE_UPLOAD : uses
Class diagram for file upload handlers and loaderclassDiagram
class BaseFileUpload {
<<abstract>>
+file
+load() str
}
class TXTFileUpload {
+load() str
}
class PDFFileUpload {
+load() str
}
class BaseFileUploader {
+files
+document_delimiter
+load() str
}
class FileUpLoader {
+__init__(file_path: str)
}
BaseFileUpload <|-- TXTFileUpload
BaseFileUpload <|-- PDFFileUpload
BaseFileUploader <|-- FileUpLoader
FileUpLoader o-- BaseFileUpload : uses
FileUpLoader o-- TXTFileUpload : uses
FileUpLoader o-- PDFFileUpload : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ieaves, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the ramalama CLI by enabling the direct ingestion of file contents into the model's prompt. This functionality is implemented through a new, modular file upload system that supports various text-based formats and is designed for future extensibility. The changes empower users to provide rich context to the model directly from their local files or project directories.
Highlights
- New Feature: File Input for
ramalama run: I've added a new--inputargument to theramalama runcommand, allowing users to pass the content of specified files or entire directories as part of the prompt to the model. - Modular File Upload System: A new
ramalama.file_uploadpackage has been introduced, featuring a modular design withBaseFileUploadandBaseFileUploaderclasses. This architecture supports easy extension for new file types. - Supported File Types: Initial support for various text-based file types (e.g.,
.txt,.md,.json,.yaml,.csv,.toml,.sh) and a placeholder for.pdffiles has been implemented. Unsupported file types are gracefully ignored with a warning. - Automated Content Inclusion: The
chatfunction now automatically loads content from the specified input files or directories and appends it to theARGS(prompt) that is sent to the model, delimited for clarity. - Comprehensive Testing: Extensive unit and integration tests have been added to validate the new file upload system, covering file loading, directory handling, unsupported types, and the integration with the
chatcommand.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ieaves - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `ramalama/file_upload/file_types/pdf.py:10` </location>
<code_context>
+ Load the content of the PDF file.
+ This method should be implemented to handle PDF file reading.
+ """
+ return ""
</code_context>
<issue_to_address>
PDFFileUpload.load currently returns an empty string, which may be misleading.
Raising NotImplementedError or issuing a warning would make it clear that PDF loading is not yet implemented, avoiding confusion with an empty PDF result.
</issue_to_address>
### Comment 2
<location> `test/unit/test_file_upload.py:187` </location>
<code_context>
+ expected = f"\n<!--start_document {tmp_file.name}-->\nTest content\nwith multiple lines"
+ assert result == expected
+
+ def test_file_uploader_load_multiple_files(self):
+ """Test loading multiple files from a directory."""
+ with tempfile.TemporaryDirectory() as tmp_dir:
</code_context>
<issue_to_address>
Consider adding a test for deeply nested directories.
Please clarify whether files in subdirectories should be processed. Add a test to verify this behavior, and update the implementation if needed.
Suggested implementation:
```python
def test_file_uploader_load_multiple_files(self):
"""Test loading multiple files from a directory."""
with tempfile.TemporaryDirectory() as tmp_dir:
txt_file = os.path.join(tmp_dir, "test.txt")
with open(txt_file, "w") as f:
f.write("Text content")
md_file = os.path.join(tmp_dir, "test.md")
with open(md_file, "w") as f:
f.write("# Markdown content")
uploader = FileUpLoader(tmp_dir)
assert len(uploader.files) == 2
def test_file_uploader_ignores_nested_directories(self):
"""Test that files in deeply nested directories are not processed (current behavior)."""
with tempfile.TemporaryDirectory() as tmp_dir:
# Create a file in the root directory
root_file = os.path.join(tmp_dir, "root.txt")
with open(root_file, "w") as f:
f.write("Root file")
# Create a nested directory structure
nested_dir = os.path.join(tmp_dir, "level1", "level2", "level3")
os.makedirs(nested_dir)
nested_file = os.path.join(nested_dir, "nested.txt")
with open(nested_file, "w") as f:
f.write("Nested file")
uploader = FileUpLoader(tmp_dir)
# Only the root file should be processed if subdirectories are ignored
assert root_file in uploader.files
assert nested_file not in uploader.files
assert len(uploader.files) == 1
```
If the intended behavior is to process files in subdirectories, you will need to update the `FileUpLoader` implementation to recursively find files. If so, let me know and I can provide the necessary implementation changes.
</issue_to_address>
### Comment 3
<location> `test/unit/test_file_upload.py:207` </location>
<code_context>
+ assert "test.txt" in result
+ assert "test.md" in result
+
+ def test_file_uploader_unsupported_file_types(self):
+ """Test handling of unsupported file types."""
+ with tempfile.TemporaryDirectory() as tmp_dir:
</code_context>
<issue_to_address>
Add a test to verify that a warning is emitted for unsupported file types.
Please add a test that asserts a warning is raised when unsupported file types are present, for example using pytest.warns.
Suggested implementation:
```python
def test_file_uploader_unsupported_file_types(self):
"""Test handling of unsupported file types."""
with tempfile.TemporaryDirectory() as tmp_dir:
# Create a supported file
txt_file = os.path.join(tmp_dir, "test.txt")
with open(txt_file, "w") as f:
f.write("Text content")
# Create an unsupported file
unsupported_file = os.path.join(tmp_dir, "test.exe")
with open(unsupported_file, "w") as f:
f.write("This should trigger a warning.")
import pytest
# Assuming the warning is a UserWarning; adjust if your code uses a custom warning
with pytest.warns(UserWarning, match="unsupported file type"):
uploader = FileUpLoader(tmp_dir)
uploader.load()
```
- Ensure that the `FileUpLoader` class emits a `UserWarning` (or a custom warning) when encountering unsupported file types. If it does not, you will need to add this behavior in the implementation.
- Adjust the warning type and match string in `pytest.warns` if your code uses a different warning class or message.
- Make sure `import pytest` is present at the top of your test file if not already imported.
</issue_to_address>
### Comment 4
<location> `test/unit/test_file_upload.py:175` </location>
<code_context>
+ with pytest.raises(ValueError, match="does not exist"):
+ FileUpLoader("/nonexistent/directory")
+
+ def test_file_uploader_load_single_text_file(self):
+ """Test loading a single text file."""
+ with tempfile.NamedTemporaryFile(suffix=".txt") as tmp_file:
</code_context>
<issue_to_address>
Consider adding a test for files with no extension.
Please add a test to confirm how files without extensions are handled—whether they are ignored or raise an error.
Suggested implementation:
```python
def test_file_uploader_load_single_text_file(self):
"""Test loading a single text file."""
with tempfile.NamedTemporaryFile(suffix=".txt") as tmp_file:
with open(tmp_file.name, "w") as f:
f.write("Test content")
uploader = FileUpLoader(tmp_file.name)
assert len(uploader.files) == 1
assert isinstance(uploader.files[0], TXTFileUpload)
assert uploader.files[0].file == tmp_file.name
def test_file_uploader_file_without_extension(self):
"""Test loading a file with no extension."""
with tempfile.NamedTemporaryFile(suffix="", delete=False) as tmp_file:
tmp_file.write(b"No extension content")
tmp_file.flush()
file_path = tmp_file.name
try:
uploader = FileUpLoader(file_path)
# If files without extension are ignored, assert that uploader.files is empty
assert len(uploader.files) == 0
except Exception as e:
# If an error is expected, assert the type of error
assert isinstance(e, ValueError)
finally:
os.remove(file_path)
```
- If `FileUpLoader` is expected to behave differently (e.g., raise a specific error or handle files without extension in a custom way), adjust the assertions accordingly.
- Ensure `os` is imported at the top of the file if not already present.
</issue_to_address>
### Comment 5
<location> `test/unit/test_file_upload_with_data.py:172` </location>
<code_context>
+ assert "sample.md" in result
+ assert "sample.json" in result
+
+ def test_unsupported_file_handling(self, data_dir):
+ """Test that unsupported files are handled correctly."""
+
</code_context>
<issue_to_address>
Consider adding a test for a file with a supported extension but unreadable permissions.
Add a test to verify that the loader handles unreadable files with supported extensions by failing gracefully or raising the correct error.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_unsupported_file_handling(self, data_dir):
"""Test that unsupported files are handled correctly."""
=======
def test_unsupported_file_handling(self, data_dir):
"""Test that unsupported files are handled correctly."""
def test_unreadable_supported_file(self, tmp_path):
"""Test that unreadable files with supported extensions are handled gracefully."""
import os
from ramalama.file_upload.file_loader import FileUpLoader
# Create a supported file (e.g., .txt)
unreadable_file = tmp_path / "unreadable.txt"
unreadable_file.write_text("This file should not be readable.")
# Remove read permissions
unreadable_file.chmod(0o000)
try:
loader = FileUpLoader()
with pytest.raises(Exception):
loader.load_file(str(unreadable_file))
finally:
# Restore permissions so pytest can clean up the file
unreadable_file.chmod(0o644)
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `ramalama/file_upload/file_loader.py:1` </location>
<code_context>
+import itertools
+import os
+from string import Template
</code_context>
<issue_to_address>
Consider removing unused imports, simplifying delimiter handling, and collapsing file discovery and instantiation logic into concise comprehensions.
```suggestion
# 1) Drop unused imports and the Template/Type machinery
- Remove `import itertools`, `from string import Template`, `from typing import Type`.
- Update BaseFileUploader to store a plain string prefix and use an f-string in load().
--- a/ramalama/file_upload/uploader.py
@@
- import itertools
- from string import Template
- from typing import Type
+ import os
+ from warnings import warn
+ from ramalama.file_upload.file_types import base, pdf, txt
@@
- class BaseFileUploader:
+ class BaseFileUploader:
@@
- def __init__(self, files: list[Type[base.BaseFileUpload]], delim_string: str = "<!--start_document $name-->"):
- self.files = files
- self.document_delimiter: Template = Template(delim_string)
+ def __init__(self, files: list[base.BaseFileUpload], delimiter_prefix: str = "<!--start_document"):
+ self.files = files
+ self.delimiter_prefix = delimiter_prefix
@@
- def load(self) -> str:
- """Generate the output string by concatenating the processed files."""
- output = (f"\n{self.document_delimiter.substitute(name=f.file)}\n{f.load()}" for f in self.files)
- return "".join(output)
+ def load(self) -> str:
+ """Generate the output string by concatenating the processed files."""
+ return "".join(
+ f"\n{self.delimiter_prefix} {f.file}-->\n{f.load()}"
+ for f in self.files
+ )
# 2) Collapse discovery, filtering, warning, instantiation in FileUpLoader
--- a/ramalama/file_upload/uploader.py
@@ class FileUpLoader(BaseFileUploader):
- def __init__(self, file_path: str):
- if not os.path.exists(file_path):
- raise ValueError(f"{file_path} does not exist.")
-
- if not os.path.isdir(file_path):
- files = [file_path]
- else:
- files = [
- os.path.join(file_path, f) for f in os.listdir(file_path)
- if os.path.isfile(os.path.join(file_path, f))
- ]
-
- extensions = [os.path.splitext(f)[1].lower() for f in files]
-
- if set(extensions) - set(SUPPORTED_EXTENSIONS):
- warning_message = (
- f"Unsupported file types found: {set(extensions) - set(SUPPORTED_EXTENSIONS)}\n"
- f"Supported types are: {set(SUPPORTED_EXTENSIONS.keys())}"
- )
- warn(warning_message)
-
- files = [SUPPORTED_EXTENSIONS[ext](file=f)
- for ext, f in zip(extensions, files)
- if ext in SUPPORTED_EXTENSIONS]
- super().__init__(files=files)
+ def __init__(self, path: str):
+ if not os.path.exists(path):
+ raise ValueError(f"{path} does not exist.")
+
+ # collect all candidate files
+ candidates = [path] if os.path.isfile(path) else [
+ os.path.join(path, name) for name in os.listdir(path)
+ if os.path.isfile(os.path.join(path, name))
+ ]
+
+ # warn on unsupported extensions
+ exts = [os.path.splitext(p)[1].lower() for p in candidates]
+ unsupported = set(exts) - set(SUPPORTED_EXTENSIONS)
+ if unsupported:
+ warn(f"Unsupported file types: {unsupported}. Supported: {set(SUPPORTED_EXTENSIONS)}")
+
+ # instantiate only supported uploads
+ instances = [
+ SUPPORTED_EXTENSIONS[ext](file=p)
+ for p in candidates
+ if (ext := os.path.splitext(p)[1].lower()) in SUPPORTED_EXTENSIONS
+ ]
+ super().__init__(instances)
```
This removes indirection, drops unused imports, replaces `Template` with a simple f-string, and collapses listing/filtering/instantiation into concise comprehensions while preserving all behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| expected = f"\n<!--start_document {tmp_file.name}-->\nTest content\nwith multiple lines" | ||
| assert result == expected | ||
|
|
||
| def test_file_uploader_load_multiple_files(self): |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for deeply nested directories.
Please clarify whether files in subdirectories should be processed. Add a test to verify this behavior, and update the implementation if needed.
Suggested implementation:
def test_file_uploader_load_multiple_files(self):
"""Test loading multiple files from a directory."""
with tempfile.TemporaryDirectory() as tmp_dir:
txt_file = os.path.join(tmp_dir, "test.txt")
with open(txt_file, "w") as f:
f.write("Text content")
md_file = os.path.join(tmp_dir, "test.md")
with open(md_file, "w") as f:
f.write("# Markdown content")
uploader = FileUpLoader(tmp_dir)
assert len(uploader.files) == 2
def test_file_uploader_ignores_nested_directories(self):
"""Test that files in deeply nested directories are not processed (current behavior)."""
with tempfile.TemporaryDirectory() as tmp_dir:
# Create a file in the root directory
root_file = os.path.join(tmp_dir, "root.txt")
with open(root_file, "w") as f:
f.write("Root file")
# Create a nested directory structure
nested_dir = os.path.join(tmp_dir, "level1", "level2", "level3")
os.makedirs(nested_dir)
nested_file = os.path.join(nested_dir, "nested.txt")
with open(nested_file, "w") as f:
f.write("Nested file")
uploader = FileUpLoader(tmp_dir)
# Only the root file should be processed if subdirectories are ignored
assert root_file in uploader.files
assert nested_file not in uploader.files
assert len(uploader.files) == 1If the intended behavior is to process files in subdirectories, you will need to update the FileUpLoader implementation to recursively find files. If so, let me know and I can provide the necessary implementation changes.
There was a problem hiding this comment.
Yes this tool should walk the subdirectories, please suggest a implementation.
There was a problem hiding this comment.
Great! Since you want the tool to walk subdirectories, your current implementation in FileUpLoader already uses os.walk, which recursively finds all files in the given directory and its subdirectories:
files = [os.path.join(root, name) for root, _, files in os.walk(file_path) for name in files]This means all files, including those in deeply nested subdirectories, will be processed if their extensions are supported.
To ensure this behavior is tested and clear, you should add a test like the following:
def test_file_uploader_processes_nested_directories(self):
"""Test that files in deeply nested directories are processed."""
with tempfile.TemporaryDirectory() as tmp_dir:
# Create a file in the root directory
root_file = os.path.join(tmp_dir, "root.txt")
with open(root_file, "w") as f:
f.write("Root file")
# Create a nested directory structure
nested_dir = os.path.join(tmp_dir, "level1", "level2", "level3")
os.makedirs(nested_dir)
nested_file = os.path.join(nested_dir, "nested.txt")
with open(nested_file, "w") as f:
f.write("Nested file")
uploader = FileUpLoader(tmp_dir)
file_paths = {f.file for f in uploader.files}
assert root_file in file_paths
assert nested_file in file_paths
assert len(file_paths) == 2This test will confirm that files in all subdirectories are included. You can add it to your TestFileUpLoader class.
Let me know if you need a test for filtering or for unsupported file types in subdirectories as well!
| assert "test.txt" in result | ||
| assert "test.md" in result | ||
|
|
||
| def test_file_uploader_unsupported_file_types(self): |
There was a problem hiding this comment.
suggestion (testing): Add a test to verify that a warning is emitted for unsupported file types.
Please add a test that asserts a warning is raised when unsupported file types are present, for example using pytest.warns.
Suggested implementation:
def test_file_uploader_unsupported_file_types(self):
"""Test handling of unsupported file types."""
with tempfile.TemporaryDirectory() as tmp_dir:
# Create a supported file
txt_file = os.path.join(tmp_dir, "test.txt")
with open(txt_file, "w") as f:
f.write("Text content")
# Create an unsupported file
unsupported_file = os.path.join(tmp_dir, "test.exe")
with open(unsupported_file, "w") as f:
f.write("This should trigger a warning.")
import pytest
# Assuming the warning is a UserWarning; adjust if your code uses a custom warning
with pytest.warns(UserWarning, match="unsupported file type"):
uploader = FileUpLoader(tmp_dir)
uploader.load()- Ensure that the
FileUpLoaderclass emits aUserWarning(or a custom warning) when encountering unsupported file types. If it does not, you will need to add this behavior in the implementation. - Adjust the warning type and match string in
pytest.warnsif your code uses a different warning class or message. - Make sure
import pytestis present at the top of your test file if not already imported.
There was a problem hiding this comment.
Great! To add this test, simply copy the suggested implementation into your TestFileUpLoader class in test_file_upload.py. Make sure that your FileUpLoader implementation actually emits a UserWarning (or another warning type) when it encounters unsupported file types—otherwise, the test will fail and you may need to update the implementation accordingly.
If you need the warning to be more specific or want to use a custom warning class, adjust the pytest.warns parameters as needed. Also, ensure import pytest is present at the top of your test file (it already is in your current file).
Let me know if you need help updating the implementation to emit the warning, or if you want to see a full code block for the test!
| with pytest.raises(ValueError, match="does not exist"): | ||
| FileUpLoader("/nonexistent/directory") | ||
|
|
||
| def test_file_uploader_load_single_text_file(self): |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for files with no extension.
Please add a test to confirm how files without extensions are handled—whether they are ignored or raise an error.
Suggested implementation:
def test_file_uploader_load_single_text_file(self):
"""Test loading a single text file."""
with tempfile.NamedTemporaryFile(suffix=".txt") as tmp_file:
with open(tmp_file.name, "w") as f:
f.write("Test content")
uploader = FileUpLoader(tmp_file.name)
assert len(uploader.files) == 1
assert isinstance(uploader.files[0], TXTFileUpload)
assert uploader.files[0].file == tmp_file.name
def test_file_uploader_file_without_extension(self):
"""Test loading a file with no extension."""
with tempfile.NamedTemporaryFile(suffix="", delete=False) as tmp_file:
tmp_file.write(b"No extension content")
tmp_file.flush()
file_path = tmp_file.name
try:
uploader = FileUpLoader(file_path)
# If files without extension are ignored, assert that uploader.files is empty
assert len(uploader.files) == 0
except Exception as e:
# If an error is expected, assert the type of error
assert isinstance(e, ValueError)
finally:
os.remove(file_path)- If
FileUpLoaderis expected to behave differently (e.g., raise a specific error or handle files without extension in a custom way), adjust the assertions accordingly. - Ensure
osis imported at the top of the file if not already present.
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to pass files to ramalama run, enhancing its utility by allowing file contents to be directly included in prompts. Key feedback points include correcting the handling of args.ARGS in ramalama/chat.py to prevent TypeErrors, refining a type hint in BaseFileUploader for better clarity, and suggesting a default encoding (utf-8) for reading text files to improve robustness.
ramalama/file_upload/file_loader.py
Outdated
| This class should be extended by specific file type handlers. | ||
| """ | ||
|
|
||
| def __init__(self, files: list[Type[base.BaseFileUpload]], delim_string: str = "<!--start_document $name-->"): |
There was a problem hiding this comment.
The type hint for the files parameter in BaseFileUploader.__init__ is list[Type[base.BaseFileUpload]]. This suggests it expects a list of classes, but a list of instances is being passed. To accurately reflect that files is a list of instances, the type hint should be list[base.BaseFileUpload].
| def __init__(self, files: list[Type[base.BaseFileUpload]], delim_string: str = "<!--start_document $name-->"): | |
| def __init__(self, files: list[base.BaseFileUpload], delim_string: str = "<!--start_document $name-->"): |
| """ | ||
|
|
||
| # TODO: Support for non-default encodings? | ||
| with open(self.file, 'r') as f: |
There was a problem hiding this comment.
The TODO comment on line 10 mentions supporting non-default encodings. Opening files without specifying an encoding uses the system's default, which can lead to UnicodeDecodeError. Using encoding='utf-8' is a robust default for text files.
| with open(self.file, 'r') as f: | |
| with open(self.file, 'r', encoding='utf-8') as f: # Specify UTF-8 encoding as a robust default |
|
Hey @rhatdan and @ericcurtin, this PR came about by way of reference to #867 and I'd love some directional feedback. The group over at pymupdf has a pure python implementation of a pdf reader with a permissive, copy-left license. If this direction looked right, we could either add it as an optional dependency or copy their current implementation over to support a pdf reader. |
|
I'm not sure we want this added dependancy, what we can do is: |
Do you mean the pdf reader as a dependency or the ability to pass file content? If the former, as I mentioned there are a few easy paths
|
|
Should we just use -v and support container volumes or --mount? Does this option make sense in the --nocontainer use case? |
|
BTW Love to keep some of these tests, but need to understand why I need to move files around at all, if the issue is just to provide file/directory access to the Model Engines within the containers, then volume mounts can do that. |
We could consider 4. , installing it inside the container and executing it there. |
|
Just as a little preamble, the way I understood the initial request is that the user was looking for a way to perform one off analysis. Something like ramalama run --input my_document.pdf tinyllama Please summarize this document for me:Would spin up an instance of tinyllama, consume the prompt and document, respond with the requested summary, and wind down. This workflow can already be accomplished for a single raw text based file by piping the content of the file into the prompt but gets a little more complicated for more complicated document formats or a directory of documents. We can definitely mount the user specified file or directory into the container but what are you thinking from there? Is the idea that we would move all of the reader code into a separate dependency that we bake into every image build and somehow trigger when the user provides the |
|
I recommend doing something like this: I used to do things like this with RamaLama before, worked great. The biggest problem is the new dependancy. But if you do it this way, we avoid that. pdf2txt is obviously a made up thing, but put a tool of your choice there. Just tried to test this with: doesn't seem to work anymore, that's a bug. |
|
Okay, it sounds like you aren't interested in Ramalama itself being able to support either multi-file upload or file parsing itself? I can close this PR in that case, do you want to close out #867 as not being of interest as well? |
|
It may be possible to add dependancies inside the container did you look at our rag solution? That could be similar to what you are looking for. |
Just to be clear, this PR doesn't add a new dependency. It does include an example of how optional dependencies can be included for users who want functionality not available in a default installation but, as discussed, even that isn't strictly necessary.
I previously asked whether I was thinking about this potential implementation correctly because it just seemed like an awfully heavy solution to a fairly trivial problem and I wanted to make sure I hadn't missed something. The user is ultimately just trying to append documents to their prompt but following the RAG model they would need to do something like this:
This is asking a lot from users to perform what could otherwise be a fairly simple task. |
|
Lets setup a google meet to further discuss the PR/Issue you are trying to solve. Once we get the requirements down we can decide if this is something to be added to RamaLama or something to be done externally and then piped into the ramalama run/ramalama chat interface. |
|
Sounds good, I don't have your email @rhatdan but I'll shoot @ericcurtin an email to see if we can find some time for a quick call this week! |
|
My email is [email protected] |
|
After meeting today, we believe the first version of this should be ramalama chat --rag /tmp/myrag --url http://localhost:8080 Where ramalama will read the markdown file in the --rag directory and add it to the context before querying the Model Runtime. Nest step would be to change ramalama rag a.md b.pdf c.docx /tmp/mynewrag Which would tell ramalama to use the docling-rag container image to convert the specified files into markdown language to be used in a new command:
Since ramalama run is now ramalama serve; ramalama chat, this should quickly get us the same support into ramalama run command. We still want to support: ramalama serve --rag OCI:quay.io/ramalama/myrag:1.2 MODEL for server side support. |
|
You need to sign all of your commits, but it might be that we need a total rewrite. of this PR. |
|
Tell me more, I can definitely change the argument from --context to --rag which would be the phase 1 we discussed (and is fairly trivial at this point). If we are simply adding the documents to the user's query, I'm not sure it's the best idea to cross contaminate the --rag argument but happy to defer to your judgement if you feel strongly on the point. Do you want to include a migration to docling in this PR as well though? If so, how do you feel about first building a new minimal image with docling for document processing and then when a user calls the chat endpoint with
|
|
Lets do the ramalama chat --rag/context first. Then we can do I am not wedded to --rag, but --context was not as clear to me what you were doing. Issue with ramama run/chat --rag is that this is on the client side. Having --context and --rag in |
|
@rhatdan if we are putting docling off for the moment, can we move forwards on this PR? Perhaps after changing from --context to --rag? |
|
Yes |
ieaves
force-pushed
the
feat/file-upload
branch
5 times, most recently
from
6bad636 to
81a8b86
Compare
|
This should be good to do using |
|
Build failure with: anyway we could fix that? |
|
Hey @ericcurtin should be good now, looks like submodules weren't being included in stricter build environments. Since I was looking at the build I threw in a fix for the deprecated license declaration behavior as well, hope that's okay! |
ramalama/file_upload/file_loader.py
Outdated
| files = [file_path] | ||
| else: | ||
| files = [ | ||
| os.path.join(file_path, f) for f in os.listdir(file_path) if os.path.isfile(os.path.join(file_path, f)) |
There was a problem hiding this comment.
Should this be a Walk rather then a single layer?
There was a problem hiding this comment.
For sure! Switched it over just now :)
|
Great test suite, this looks very good, I have a few comments and then we can merge. Love to have a demo of this or a blog once it works and is released. |
Signed-off-by: Ian Eaves <[email protected]>
Awesome, thanks Dan! Once it's merged I'll put something together we can use for both. It might be a good tie in to some of the RAG features y'all have been working on as well since this could be a sort of low stakes entree to those capabilities. I didn't see any other comments on the PR though, did you want to throw those into a review or ping me another way? Happy to address whatever you're thinking about. |
|
LGTM I swear I reviewed a version of this where you were not using the walk function, and I said to walk the specified directory. Now I see that walk is here. There is a sourcery suggestion to add a couple more tests, but this is ready to merge, so I will merge. BTW Did you use AI to generate the tests suite? |
Definitely yes for the scaffolding, I've found it's very helpful generating much of the boilerplate and ideating on potential failure paths but much, much worse at actually implementing meaningful tests without supervision. For this PR I, asked it to generate a test suite but found the test implementations somewhat trivial. I had to go back and provide targeted guidance on a better approach and with those additional instructions it mostly fixed things through in the subsequent pass. After that I individually reviewed each and made targeted changes as needed. It's a huge time saver though for generating all of the tedious mocks and what not. |
Summary by Sourcery
Enable users to pass files or directories into the ramalama commands by introducing a new --input flag, implementing a file upload loader with supported file-type handlers, integrating file contents into chat prompts, and covering the functionality with extensive tests.
New Features:
Enhancements:
Tests: