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.

Sign up for GitHub

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

Jump to bottom

OpenAI Chat & Assistant hook functions #38736

Merged
merged 5 commits into from
Apr 19, 2024
Merged

OpenAI Chat & Assistant hook functions #38736

merged 5 commits into from
Apr 19, 2024

Conversation

nathadfield
Copy link
Collaborator

Adding a set of additional hook functions for the OpenAI provider that deal with interactions between some of the Chat and Assistants API endpoints.

@kaxil kaxil requested review from utkarsharma2, pankajkoti and Lee-W and removed request for utkarsharma2 and pankajkoti April 4, 2024 11:46
@nathadfield nathadfield force-pushed the openai_hooks branch 7 times, most recently from 4d9977a to 9b67e5c Compare April 5, 2024 13:47
@nathadfield nathadfield changed the title OpenAI hook functions OpenAI Chat & Assistant hook functions Apr 5, 2024
@nathadfield nathadfield marked this pull request as ready for review April 5, 2024 16:16
@nathadfield nathadfield force-pushed the openai_hooks branch 3 times, most recently from 33248aa to 8d42b77 Compare April 8, 2024 08:07
@nathadfield
Copy link
Collaborator Author

I'd love a review on this when someone gets an opportunity. Thanks!

Lee-W
Lee-W reviewed Apr 8, 2024
View reviewed changes
airflow/providers/openai/hooks/openai.py Outdated Show resolved Hide resolved
airflow/providers/openai/hooks/openai.py Outdated Show resolved Hide resolved
@teowave
Copy link

teowave commented Apr 9, 2024

Excellent and timely addition. I was just struggling to get openai chat completions to work straight from within pythonoperator, only to silently hang. Looking forward to seeing this implemented

@nathadfield
Copy link
Collaborator Author

One failed test here but I don't think it's anything to do with my changes.

@Lee-W
Copy link
Member

Lee-W commented Apr 9, 2024

One failed test here but I don't think it's anything to do with my changes.

Just retrigger it. Let's see how it works

@nathadfield
Copy link
Collaborator Author

nathadfield commented Apr 9, 2024 •
edited
Loading

I rebased which ran the checks again but it's now failing with a different error but still not connected to my changes. How else can I retrigger the checks?

@nathadfield nathadfield force-pushed the openai_hooks branch 2 times, most recently from 0063d3c to 4e1a9e4 Compare April 10, 2024 06:56
@nathadfield
Copy link
Collaborator Author

@Lee-W I keep rebasing and rerunning but some unrelated tests always seem to fail. Any suggestions?

@Lee-W
Copy link
Member

Lee-W commented Apr 10, 2024

Looks like network issue 🤔 let me try again. if it still fails, I'll take a deeper look

@nathadfield
Copy link
Collaborator Author

Looks like network issue 🤔 let me try again. if it still fails, I'll take a deeper look

You seem to have the magic touch!

@Lee-W Lee-W self-requested a review April 10, 2024 14:22
@nathadfield
Copy link
Collaborator Author

Just looking for someone to give it a 👍 now.

Lee-W
Lee-W reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. Left a few nitpicks. But would like have @utkarsharma2 to take another look to ensure I did not miss anything crucial 🙂

airflow/providers/openai/hooks/openai.py Outdated Show resolved Hide resolved
airflow/providers/openai/hooks/openai.py Outdated Show resolved Hide resolved
tests/providers/openai/hooks/test_openai.py Outdated Show resolved Hide resolved
@nathadfield
Copy link
Collaborator Author

@Lee-W No worries. Thanks for the input.

@utkarsharma2 It'd be great to get any more thoughts from you.

@nathadfield
Copy link
Collaborator Author

Is someone able to merge this soon then?

@Lee-W
Copy link
Member

Lee-W commented Apr 19, 2024

I'm planning on merging this one later today. Please let me know if anyone want to take a deeper look.

@Lee-W Lee-W merged commit 2674a69 into apache:main Apr 19, 2024
92 checks passed
@nathadfield nathadfield deleted the openai_hooks branch April 24, 2024 10:55
@eladkal eladkal mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pankajkoti pankajkoti pankajkoti approved these changes

@Lee-W Lee-W Lee-W approved these changes

@utkarsharma2 utkarsharma2 Awaiting requested review from utkarsharma2

Assignees
No one assigned
Labels
area:providers provider:openai
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nathadfield @teowave @Lee-W @pankajkoti @utkarsharma2