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

Anthropic tool support #370

Merged
merged 3 commits into from
May 7, 2024
Merged

Anthropic tool support #370

merged 3 commits into from
May 7, 2024

Conversation

SmittieC
Copy link
Collaborator

Langchain is complaining about the pace where we add a system message with the date time. Appartently the system message must be the first message. For OpenAI agents this is not the case, so might be anthropic specific. Anycase, I'm thinking of just appending "the current date is {current_date}" to the prompt

@proteusvacuum
Copy link
Collaborator

I think this is Anthropic specific #211

@SmittieC
Copy link
Collaborator Author

I think this is Anthropic specific #211

Ah, excellent!

@bderenzi
Copy link
Collaborator

Also, while OpenAI doesn't reject it, I believe I remember them saying we should not do that. I suspect all (or the vast majority) of instruction tuning is focused on the system prompt at the top of the document, so we shouldn't expect it to work if we inject an additional system prompt mid-conversation. I'm +1 on rewriting everything to have system prompt first, always.

Base automatically changed from cs/upgrade_langchain to main April 17, 2024 14:42
@SmittieC SmittieC force-pushed the cs/anthropic_tool_support branch from a0ff29f to fcb9cbd Compare May 6, 2024 12:19
@@ -185,7 +184,8 @@ def _build_chain(self) -> Runnable[dict[str, Any], Any]:

@property
def prompt(self):
system_prompt = SystemMessagePromptTemplate.from_template(self.experiment.prompt_text)
prompt = self.experiment.prompt_text + "\nCurrent datetime: {current_time}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While this is in my opinion not the most elegant solution, I can't think of another one except using a human message. I don't think we should use a human message for this though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No necessary to change right now but I think we should expand this a bit more.

Today's datetime on UTC time 2023-05-02T10:00:00+00:00, it's Tuesday.

@SmittieC SmittieC marked this pull request as ready for review May 6, 2024 12:27
@@ -215,6 +215,7 @@ def _build_chain(self) -> Runnable[dict[str, Any], str]:
{"input": RunnablePassthrough()}
| RunnablePassthrough.assign(source_material=RunnableLambda(lambda x: self.source_material))
| RunnablePassthrough.assign(participant_data=RunnableLambda(lambda x: self.participant_data))
| RunnablePassthrough.assign(current_time=RunnableLambda(lambda x: str(timezone.now())))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI: Only the agents will be using the knowlegde of the current time, but I think it would be nice if we can make all our gpt's time aware

@SmittieC
Copy link
Collaborator Author

SmittieC commented May 6, 2024

In a future PR we can update the tool to use our ScheduledMessages

@@ -185,7 +184,8 @@ def _build_chain(self) -> Runnable[dict[str, Any], Any]:

@property
def prompt(self):
system_prompt = SystemMessagePromptTemplate.from_template(self.experiment.prompt_text)
prompt = self.experiment.prompt_text + "\nCurrent datetime: {current_time}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

No necessary to change right now but I think we should expand this a bit more.

Today's datetime on UTC time 2023-05-02T10:00:00+00:00, it's Tuesday.

@@ -45,3 +45,4 @@ django-field-audit>=1.2.7
turn-python>=0.2.0
jinja2
django-taggit
transformers
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do we need this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this:

Could not import transformers python package. This is needed in order to calculate get_token_ids. Please install it with pip install transformers.

that originated from here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻 could you add a comment

snopoke
snopoke previously approved these changes May 7, 2024
@SmittieC SmittieC merged commit cc2dc4f into main May 7, 2024
4 checks passed
@SmittieC SmittieC deleted the cs/anthropic_tool_support branch May 7, 2024 13:14
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