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 Vector destination tests and move to Poetry #35911

Merged
merged 21 commits into from Mar 26, 2024

Conversation

Copy link

vercel bot commented Mar 8, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 23, 2024 3:09pm

@aaronsteers
Copy link
Collaborator Author

aaronsteers commented Mar 8, 2024

Milvus account has reached OpenAI quota:

  • Job result: RateLimitError: You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors

Update: Weaviate is showing the same issue. (OpenAI quota exceeded.)

@aaronsteers
Copy link
Collaborator Author

aaronsteers commented Mar 8, 2024

Re: Pinecone failures looks like breaking changes in the pinecone library. Probably need to pin to prior version:

  • TypeError: <lambda>() got an unexpected keyword argument 'replicas'
  • AttributeError: init is no longer a top-level attribute of the pinecone package.�Please create an instance of the Pinecone class instead.�Example:�import os; from pinecone import Pinecone, ServerlessSpec; pc = Pinecone(api_key=os.environ.get("PINECONE_API_KEY"))... pc.create_index(...

Update: I've pinned to <3.0. For future ref, migration guide is here.

@aaronsteers aaronsteers marked this pull request as ready for review March 8, 2024 06:46
@aaronsteers aaronsteers requested a review from a team as a code owner March 8, 2024 06:46
@aaronsteers
Copy link
Collaborator Author

I've updated the configs in GSM so that each of these three uses embeddings "mode": "fake" instead of "mode": "openai".

Copy link
Contributor

@bindipankhudi bindipankhudi left a comment

Choose a reason for hiding this comment

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

Besides the OpenAI quota issues, are these all the changes we need to comply with the new connector requirements?

@aaronsteers aaronsteers changed the title Vector Destination Patches Vector Destination Patches (working draft, do not merge) Mar 8, 2024
@aaronsteers
Copy link
Collaborator Author

Are these all the changes we need to comply with the new connector requirements? Will these changes make the pre-release tests pass ?

As of now I'm using this PR as a placeholder and vehicle to run/debug CI tests.

It only contains one fix as of now (the pinecone library pin) but the idea is that before merging we'll get all passing tests again.

@bindipankhudi
Copy link
Contributor

I've updated the configs in GSM so that each of these three uses embeddings "mode": "fake" instead of "mode": "openai".

assuming we are good for now then?

@aaronsteers
Copy link
Collaborator Author

I've updated the configs in GSM so that each of these three uses embeddings "mode": "fake" instead of "mode": "openai".

assuming we are good for now then?

Still a bit more work to do. One connector (Vectara, I think) went green after this. Another (pinecone) appears to need a more specific version to be pinned, and the third (weaviate) had some integration tests that strictly required the OpenAI key.

All of these also have the (new) requirement to be on Poetry. If we don't fix that poetry failure, we'll be blind to other failure types because status will stay constantly red.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ aaronsteers
❌ bindipankhudi


bindipankhudi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bindipankhudi bindipankhudi changed the title Vector Destination Patches (working draft, do not merge) Fix Vector destination tests and move to Poetry Mar 23, 2024
@bindipankhudi
Copy link
Contributor

Tests for Milvus, Pinecone, and Weaviate destinations are green now.

Changes:

  • Poetry configuration for the destinations above
  • Open AI key updated for all destinations (no longer using faker)
  • Milvus cluster re-enabled - this was causing the Milvus tests to fail

Copy link
Collaborator Author

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

✅ Approved!

This is huge to have these passing tests again!

Two points for follow-up, but non-blocking:

  1. The run.run CLI references in the pyproject.toml files won't work yet, but also they shouldn't hurt anything in the meanwhile.
  2. It's probably worth looping back sometime soon to bump the CDK versions to latest. The version here is 57ish in most cases, whereas I think the latest version is around 75 now.

I think we can handle both in future cycles.

@bindipankhudi bindipankhudi merged commit 3fe750f into master Mar 26, 2024
31 of 32 checks passed
@bindipankhudi bindipankhudi deleted the aj/vector-db-patches branch March 26, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants