Conversation
PedramNavid
left a comment
PedramNavid
left a comment
There was a problem hiding this comment.
Approved, just a small suggestion on one line.
|
@cmosguy there's a few conflicts that need to be resolved. i think you can just pull from the main branch for both pyproject and uv.lock but ensure your ipython notebook has run with ruff format and ruff check. |
|
@PedramNavid - trying again, thanks for the heads up |
PedramNavid
left a comment
PedramNavid
left a comment
There was a problem hiding this comment.
Hi @cmosguy
I went through the full PR. There's a few issues
- Rather than import tokenizer, lets rely on the count tokens API already available in the Anthropic library
- Please ensure you are using the ruff formatter, as there are a lot of changed lines here that are pure formatting changes that should not be part of the diff
- Please do not make changes to the uv.lock file, we should not be making any dependency changes for this PR.
| " def process_raw_search_results(\n", | ||
| " self,\n", | ||
| " results: list[SearchResult],\n", | ||
| " self, results: list[SearchResult],\n", |
There was a problem hiding this comment.
Have you run ruff format on this notebok? This change you made undoes what our formatter is doing.
| " result = \"\\n\".join(\n", | ||
| " [\n", | ||
| " f'<item index=\"{i + 1}\">\\n<page_content>\\n{r}\\n</page_content>\\n</item>'\n", | ||
| " f'<item index=\"{i+1}\">\\n<page_content>\\n{r}\\n</page_content>\\n</item>'\n", |
There was a problem hiding this comment.
Same here, looks like your linter/formatter is not using the ruff format we use.
| "class WikipediaSearchResult(SearchResult):\n", | ||
| " title: str\n", | ||
| "\n", | ||
| "from transformers import AutoTokenizer\n", |
There was a problem hiding this comment.
Rather than adding a new dependency, I think we should use the messages.count_tokens API.
| " page = wikipedia.page(result)\n", | ||
| " print(page.url)\n", | ||
| " except Exception:\n", | ||
| " except:\n", |
There was a problem hiding this comment.
should not have bare exceptions
| "# load the antrophic key from .env\n", | ||
| "from dotenv import load_dotenv\n", | ||
| "load_dotenv(verbose=True)\n", | ||
| "ANTHROPIC_SEARCH_MODEL = os.environ.get('ANTHROPIC_MODEL', 'claude-2')\n", |
There was a problem hiding this comment.
Let's default to haiku 4-5
uv.lock
Outdated
| [[package]] | ||
| name = "huggingface-hub" | ||
| version = "1.0.0" | ||
| version = "0.36.0" |
There was a problem hiding this comment.
should not be changing our existing dependencies to a lower version.
|
@PedramNavid gentle ping here on the updates. Do they meet your requirements now? |
PedramNavid
left a comment
PedramNavid
left a comment
There was a problem hiding this comment.
Hi @cmosguy. I've given it another look, there's quite a few issues still with this PR. I think part of the challenge is that was an old notebook from Claude 2 and so is mixing a lot of old and new concepts. I wonder if maybe a re-write might be better than trying to fix things piece meal. Either way, I've noted a few logic issues that would need to be resolved before we can merge.
| "voyageai>=0.3.5", | ||
| "python-dotenv>=1.1.1", | ||
| "wikipedia>=1.4.0", | ||
| "huggingface-hub>=1.0.0", |
There was a problem hiding this comment.
Can delete this I imagine
There was a problem hiding this comment.
I think we should leave in wikipedia, right as that is required in this notebook
| "\n", | ||
| " def __init__():\n", | ||
| " def __init__(self, anthropic_client: Anthropic):\n", | ||
| " self.anthropic_client = anthropic_client\n", |
There was a problem hiding this comment.
Why did you add the client here?
__init__ now takes anthropic_client: Anthropic as a required parameter, but then has a pass statement that does nothing with it.
Either remove the pass or properly initialize self.anthropic_client = anthropic_client.
| "# Create a searcher\n", | ||
| "wikipedia_search_tool = WikipediaSearchTool()\n", | ||
| "ANTHROPIC_SEARCH_MODEL = \"claude-2\"\n", | ||
| "# load the antrophic key from .env\n", |
There was a problem hiding this comment.
can delete this comment, the loading happens at cell 3 with `load_dotenv()
| " )\n", | ||
| " print(partial_completion)\n", | ||
| " token_budget -= self.count_tokens(partial_completion)\n", | ||
| " token_count = self.messages.count_tokens(\n", |
There was a problem hiding this comment.
I'm not sure this is correct.
You call self.messages.count_tokens() after every partial completion to count the prompt tokens, not the completion tokens.
This is backwards - you should be subtracting partial_completion_.usage.input_tokens + partial_completion_.usage.output_tokens from the budget, which are already returned by the Messages API.
| " )\n", | ||
| " information = extract_between_tags(\"information\", retrieval_response)[-1]\n", | ||
| "\n", | ||
| " # Try to extract information tags, handle case where none exist\n", |
There was a problem hiding this comment.
When <information> tags are missing you use the entire retrieval response as information. This could include all the scratchpad content and search quality reflections. Any reason why this is necessary? If no tags are found I would think there's an error in the response
| " self.search_tool = search_tool\n", | ||
| " self.verbose = verbose\n", | ||
| "\n", | ||
| " # Pass the anthropic client to the search tool if it supports it\n", |
There was a problem hiding this comment.
why wouldnt the search tool support it? You've updated the definition.
| " if search_query is None:\n", | ||
| " raise Exception(\n", | ||
| " \"Completion with retrieval failed as partial completion returned mismatched <search_query> tags.\"\n", | ||
| " f\"Completion with retrieval failed as partial completion returned mismatched <search_query> tags.\"\n", |
There was a problem hiding this comment.
why is this an f string?
2 participants
There were some changes in the api for that needed to be updated in the wikipedia notebook example. We need to start using the transformers library for the tokenizer.