-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrade existing operations to Milvus API v2 #10
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
public function __construct( | ||
protected ?string $dbName = null, | ||
) { | ||
$this->body()->setJsonFlags(JSON_FORCE_OBJECT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to set this flag, because when there isn't any parameter given, the defaultBody array is empty. And when Saloon sends it to Milvus, the default json encoded request body is []
. Milvus returns errors because it requires an object.
expect($response->json('code'))->toEqual(200); | ||
// Status 0 and empty data is a success response | ||
expect($response->status())->toEqual(200); | ||
expect($response->json('code'))->toEqual(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that code 0
is now a success response, per the documentation.
E.g. https://milvus.io/api-reference/restful/v2.5.x/v2/Collection%20(v2)/List.md
@@ -113,30 +120,33 @@ | |||
$this->milvus->collections()->create( | |||
collectionName: 'collection_test', | |||
dimension: 128, | |||
// L2 sets the distance between 0 and ∞ (0 is the closest) | |||
metricType: 'L2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any reference that the metric type has changed between versions. But when I ommit this parameter, the distance in the assertion is 1. My assumption is that the default metric type changed to something like cosine.
Different metric types and their distance value ranges: https://milvus.io/docs/metric.md
Added Github actions to test (PHP 8.2 and 8.3) and analyse. https://github.com/wiljanslofstra/milvus/actions/runs/12536214078 |
Great work! ⭐ I see some of the tests are failing locally (might be because it does not "reset" the database after each test or runs in parallel by default on my machine), will look closer into that later. Mind adding php 8.1 to the test workflow file as well?, I think it (8.1) should work without any code modification, but have not tried it myself, if it causes issues just ignore it for now. |
c06e29d
to
97ea5cd
Compare
bc459f6
to
34ef8a6
Compare
@HelgeSverre Thanks! I've added PHP 8.1 and 8.4 to the Github actions matrix for testing. I had to downgrade the PHP version in the composer.json, but that doesn't cause any issues with the other dependencies. https://github.com/wiljanslofstra/milvus/actions/runs/12620996795 |
Was this good to go @wiljanslofstra ? |
@HelgeSverre It is 🙏 |
Source: https://milvus.io/api-reference/restful/v2.5.x/About.md
I've updated the operations and resources to support the Milvus V2 API (https://milvus.io/api-reference/restful/v2.5.x/About.md).
A few notable changes:
filter
, instead of anid
.filter
field.vector
todata
.params
has becomesearchParams
andannsField
is required (for example "vector").primaryField
toprimaryFieldName
andvectorField
tovectorFieldName
.id
(added in tests).This is definitely a breaking change.
When the above changes are fine to be merged. I might add a few new endpoints, most notable the database endpoints, since I see a few issues mentioning it.