feature: remove _ClusterBatch #1259
Conversation
|
Great to see you again! Thanks for the contribution. |
|
1 test error in integration_v3/test_batch.py::test_add_1000_objects_with_async_indexing_and_wait, the rest is because the workflows were cancelled. Probably some timing issue, because code change does not affect v3, and it works locally. |
|
Hi @tsmith023, what do you think, is this the right approach, please? |
integration/test_cluster.py
Outdated
| def client(): | ||
| client = weaviate.connect_to_local() | ||
| client.collections.delete_all() | ||
| yield client | ||
| client.collections.delete_all() |
There was a problem hiding this comment.
You need to call client.close() once done with it so that resources can be cleaned up gracefully, cheers!
There was a problem hiding this comment.
you cannot delete all other classes here - other tests are running concurrently and you will delete their data while the test is running.
v3 tests are running sequentially, so it is not a problem there.
The flow should be:
- delete collection for specific test (in case there were leftovers)
- create collection
- do test things
- delete collection
There was a problem hiding this comment.
Thanks for feedback, I was not aware that tests run in parallel in v4, refactored accordingly.
integration/test_cluster.py
Outdated
| server_is_at_least_124 = parse_version_string( | ||
| client.get_meta()["version"] | ||
| ) > parse_version_string("1.24") |
There was a problem hiding this comment.
The client has this capability as client._connection._weaviate_version.is_at_least(1, 24, 0) so it would be preferable to use that here, cheers!
| async def rest_nodes( | ||
| self, | ||
| collection: Optional[str] = None, | ||
| output: Optional[Literal["minimal", "verbose"]] = None, | ||
| ) -> List[NodeREST]: |
There was a problem hiding this comment.
Since you are adding this method to _ClusterAsync, you also need to add its synchronous equivalent signature to the sync stubs in collections/cluster/sync.pyi alongside the stubs for the def nodes(...) method, cheers!
|
@tibor-reiss, removing |
|
@tibor-reiss, the problem with all the tests failing is actually an issue with our CI pipeline. We'll try to get it fixed ASAP to unblock you (and us too) |
tibor-reiss
force-pushed
the
feat_1097_cluster_batch
branch
from
9f157d8
to
afaec6f
Compare
|
@tsmith023 Sorry, I made a silly mistake removing the docker file - these rerank images did not come up locally, so I deleted it for local testing (together with the port change you also mentioned) and forgot to uncommit. I have put it back now for this PR. Though if it's not needed, it would definitely make sense to remove it :) |
|
@tibor-reiss, the fix for the broken CI jobs has been merged into |
tibor-reiss
force-pushed
the
feat_1097_cluster_batch
branch
from
afaec6f
to
ecb1949
Compare
Master is merged now 🤞 |
tibor-reiss
force-pushed
the
feat_1097_cluster_batch
branch
from
ecb1949
to
3ac374b
Compare
|
@tsmith023 friendly reminder: please check if this works (rebased onto main again just now) |
|
@tsmith023 hmmm, still timing out it seems... |
tibor-reiss
force-pushed
the
feat_1097_cluster_batch
branch
from
3ac374b
to
7bb00eb
Compare
|
Friendly ping @tsmith023 @dirkkul |
Fixes #1097
Copied also the test_cluster.py from integration_v3