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

Fix openai 1.32 breaking openai tests #40110

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 7, 2024

The new openai release adds new required parameter parallel_tool_calls and our mock did not have it. Bumping version and adding the parameter should solve the problem.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@nathadfield
Copy link
Collaborator

@potiuk Thanks! They are issuing version updates to their library on an almost daily basis currently.

@potiuk potiuk force-pushed the fix-openai-new-release-breaking-tests branch from b2d148a to d079002 Compare June 7, 2024 08:43
@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

@potiuk Thanks! They are issuing version updates to their library on an almost daily basis currently.

So if it is going to happen more frequently, maybe we will have to change our mocking strategy to be more flexible ?

@nathadfield
Copy link
Collaborator

@potiuk Thanks! They are issuing version updates to their library on an almost daily basis currently.

So if it is going to happen more frequently, maybe we will have to change our mocking strategy to be more flexible ?

Possibly. Or be stricter with the version specification?

@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

Possibly. Or be stricter with the version specification?

No. We have the rule that we do not keep upper-binding for dependencies unless we know future versions will cause us problems (sqlalchemy, flask etc.) and when we know that that the dependency follows some rule for breaking changes (mostly SemVer).

In this case - we do not know if the future change will be breaking - in fact it does not break "user" code only our tests are broken, so limiting our users from upgrading to newer releases is wrong (for example this will force us to release a new version of provider when there is a security fix released by openai. Generally speaking we are well protected by our constraints so upper-binding is not needed.

In this case if could have an approach that the mocks are always right (not sure if possible that would be best - maybe somewhere in openai library they have a way to create an "empty" Run object?

@nathadfield
Copy link
Collaborator

Understood. I'll see what I can do.

@potiuk potiuk force-pushed the fix-openai-new-release-breaking-tests branch 2 times, most recently from 4269fc0 to c2b95e0 Compare June 7, 2024 09:16
@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

It turns out we need to add a bit more limits to google and facebook provider to make sure the lower-direct limits work (otherwise it attempts to build old versions of libraries that fail).

@potiuk potiuk force-pushed the fix-openai-new-release-breaking-tests branch 2 times, most recently from 4cb232a to 7ea753d Compare June 7, 2024 13:23
@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

Hah it turned out that it is likely a bug introduced in uv==0.2.6 and for now we should limit uv to 0.2.5

See astral-sh/uv#4136 for details

The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136
@potiuk potiuk force-pushed the fix-openai-new-release-breaking-tests branch from 7ea753d to fc19cca Compare June 7, 2024 16:19
@potiuk potiuk requested a review from ashb as a code owner June 7, 2024 16:19
@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

I was too eager to upgrade UV this morning 🤦 .... The bug in uv crossed with attempts to fix the dependencies (@hussein-awala - this is why earlier I warned to be a bit cautious when upgrading to newer UV versions - but apparently I was caught up off-guard as I did it myself :(

@potiuk
Copy link
Member Author

potiuk commented Jun 7, 2024

Yep. Pinning UV to 0.2.5 fixes the problem.

@potiuk potiuk merged commit 65dbf86 into apache:main Jun 7, 2024
105 of 106 checks passed
@potiuk potiuk deleted the fix-openai-new-release-breaking-tests branch June 7, 2024 17:44
jannisko pushed a commit to jannisko/airflow that referenced this pull request Jun 15, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136
@ephraimbuddy ephraimbuddy added this to the Airflow 2.9.3 milestone Jul 1, 2024
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 1, 2024
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136

(cherry picked from commit 65dbf86)
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136

(cherry picked from commit 65dbf86)
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136

(cherry picked from commit 65dbf86)
ephraimbuddy pushed a commit that referenced this pull request Jul 2, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136

(cherry picked from commit 65dbf86)
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
The new openai release adds new required parameter `parallel_tool_calls`
and our mock did not have it. Bumping version and adding the parameter
should solve the problem. It turned out also that uv==0.2.6 introduced
a bug for lowest-direct resolution, so we need to limit it to 0.2.5.

Details in astral-sh/uv#4136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:openai
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants