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

Ragdaemon7 #578

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

Ragdaemon7 #578

wants to merge 2 commits into from

Conversation

granawkins
Copy link
Member

Pull Request Checklist

  • Documentation has been updated, or this change doesn't require that

Copy link

@mentatai mentatai bot left a comment

Choose a reason for hiding this comment

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

The pull request introduces several key changes, particularly in dependency updates and configuration handling. It's crucial to ensure that these changes are thoroughly tested, especially where they affect the system's core functionalities like file handling and daemon configuration. Overall, the updates seem to align with a general upgrade path but require careful consideration to avoid introducing new issues.

Butler is in closed beta. Reply with feedback or to ask Butler to review other parts of the PR. Please give feedback with emoji reacts.

@@ -38,7 +38,7 @@ async def run_auto_context_benchmark(sample: Sample, config: Config, cwd: Path |
ignore_patterns = [cwd / ".venv"]
annotators = {
"hierarchy": {"ignore_patterns": ignore_patterns},
"chunker_line": {"lines_per_chunk": 100},
"chunker": {},
Copy link

Choose a reason for hiding this comment

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

The change from chunker_line to chunker might affect how the data is processed. Can you confirm if this change was intended to align with updates in the ragdaemon package? It's important to ensure that this doesn't unintentionally alter the benchmarking process.

gitignore_contents = gitignore_path.read_text()
if ".ragdaemon/*" not in gitignore_contents:
gitignore_path.write_text(gitignore_contents + ".ragdaemon/*\n")

test_executable = None
Copy link

Choose a reason for hiding this comment

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

Removing the .gitignore management for .ragdaemon might expose us to potential issues with untracked files getting into version control. Was there a specific reason for removing these lines? If it's about simplifying the setup, we might want to consider other ways to handle this, possibly at a higher level in the project configuration.

@@ -75,16 +71,14 @@ async def refresh_daemon(self):

annotators: dict[str, dict[str, Any]] = {
"hierarchy": {"ignore_patterns": [str(p) for p in self.ignore_patterns]},
"chunker_line": {"lines_per_chunk": 50},
"chunker": {},
Copy link

Choose a reason for hiding this comment

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

The removal of graph_path and model from the Daemon initialization could lead to issues if the daemon relies on these parameters for specific functionalities. Please ensure that the daemon's new version is compatible with these changes and that no functionality is lost.

@@ -9,12 +9,12 @@ openai==1.13.3
pillow==10.1.0
prompt-toolkit==3.0.39
Pygments==2.15.1
ragdaemon==0.2.12
ragdaemon~=0.7.0
Copy link

Choose a reason for hiding this comment

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

Updating ragdaemon and spiceai packages could introduce breaking changes or deprecations. It's crucial to verify that all functionalities integrated with these packages are still compatible after the updates.

@@ -213,7 +213,7 @@ def func_4(string):
mock_session_context.config.auto_context_tokens = 8000

code_message = await code_context.get_code_message(0, prompt="prompt")
assert mock_session_context.llm_api_handler.spice.count_tokens(code_message, "gpt-4", is_message=True) == 85 # Code
assert mock_session_context.llm_api_handler.spice.count_tokens(code_message, "gpt-4", is_message=True) == 89 # Code
Copy link

Choose a reason for hiding this comment

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

The expected token count has changed from 85 to 89. It's good to see that tests are updated to reflect changes in the system's behavior, but it would be beneficial to add a comment explaining why this change is expected for future reference.

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.

1 participant