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

[experimental]: minor fix to open assistants code #24682

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

isahers1
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Jul 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2024 5:40pm

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 25, 2024
@dosubot dosubot bot added the 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs label Jul 25, 2024
Copy link

@bengladwell bengladwell left a comment

Choose a reason for hiding this comment

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

I think this code might lead to a different error.

A test that covers this case might be wise.

Comment on lines +624 to +627
if run.required_action:
required_tool_call_ids = {
tc.id for tc in run.required_action.submit_tool_outputs.tool_calls
}

Choose a reason for hiding this comment

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

In the case when required_action is None, doesn't that leave required_tool_call_ids unbound, which will result in an error on line 631:

if action.tool_call_id in required_tool_call_ids

in the list comprehension?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Just added code to fix that error - do you mind adding a test case and I can review and then merge?

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 26, 2024
@isahers1
Copy link
Collaborator Author

@bengladwell Do you have time to add the test? I can do later if not.

@bengladwell
Copy link

Thanks @isahers1 - I'm in the weeds on some things at the moment. I was hoping to get to this late in the week. If it would be better to get to this sooner, then yeah, that would be great if you would take it.

@isahers1
Copy link
Collaborator Author

@bengladwell Later in the week is fine, am busy with some other stuff rn so will take a look again start of next week.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 15, 2024
@isahers1 isahers1 enabled auto-merge (squash) August 15, 2024 17:40
@isahers1 isahers1 merged commit 5150ec3 into master Aug 15, 2024
53 checks passed
@isahers1 isahers1 deleted the isaac/openaiassistantsfix branch August 15, 2024 17:50
olgamurraft pushed a commit to olgamurraft/langchain that referenced this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants