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

[ref] simplify astra collection cleanup #250

Closed
wants to merge 1 commit into from

Conversation

jordanrfrazier
Copy link
Collaborator

We still see some errors like

ValueError: API Exception while running bulk insertion: {'errors': [{'message': 'Collection does not exist, collection name: documents_b00f7c83', 'errorCode': 'COLLECTION_NOT_EXIST'}]}

I don't think this PR will fix those (if they even are a result of our test cleanup logic), but this should simplify the cleanup a bit anyway, and might help since it removes at least one unnecessary delete_collection call.

@jordanrfrazier jordanrfrazier marked this pull request as ready for review January 29, 2024 23:23
@@ -233,4 +229,4 @@ def before_test(self) -> AstraDBVectorStoreTestContext:
return AstraDBVectorStoreTestContext(self)

def after_test(self):
self.ensure_astra_env_clean(blocking=False)
self.delete_collection_handler.run_delete(self.astra_ref.collection)
Copy link
Contributor

Choose a reason for hiding this comment

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

some tests use more than one collection, for example for the chat memory

@@ -201,23 +201,19 @@ def try_delete_with_backoff(self, collection: str, sleep=1, max_tries=5):
self.try_delete_with_backoff(collection, sleep * 2, max_tries)

def ensure_astra_env_clean(self, blocking=False):
logging.info("Ensuring astra env is clean")
Copy link
Contributor

Choose a reason for hiding this comment

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

the problem with this approach is that if a collection deletion is in progress, the get_collections won't return it. so we could end up with the same issue we have now

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.

2 participants