-
-
Notifications
You must be signed in to change notification settings - Fork 149
feat: S3 Vectors pkg #2017
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
feat: S3 Vectors pkg #2017
Conversation
|
I get these errors when adding some operations: GetVectors/ListVectors/PutVectors/QueryVectors ->
|
675db63 to
b9b6383
Compare
|
Thanks for this addition, I have few comments
This should be fixed by #2018
Yes, we need to fix that. #1900 |
manifest.json
Outdated
| "CreateIndex", | ||
| "CreateVectorBucket", | ||
| "DeleteIndex", | ||
| "DeleteVectorBucket", | ||
| "DeleteVectorBucketPolicy", | ||
| "DeleteVectors", | ||
| "GetIndex", | ||
| "GetVectorBucket", | ||
| "GetVectorBucketPolicy", | ||
| "ListIndexes", | ||
| "ListTagsForResource", | ||
| "ListVectorBuckets", | ||
| "PutVectorBucketPolicy", | ||
| "TagResource" |
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.
Do you need all these operations?
The philosophy of this project is to add only what is asked/needed by people, not to add everything.
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.
Removed some of them related to tagging/policies, for the store implementation i would need create bucket/index functionality
b394114 to
ea30832
Compare
|
Related to tests:
|
|
Also had to ignore |
| name: S3Vectors | ||
| package: async-aws/s3-vectors | ||
| --- | ||
|
|
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.
Please write the content of the documentation page.
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.
Done
phpstan.neon.dist
Outdated
|
|
||
| ignoreErrors: | ||
| - identifier: missingType.iterableValue | ||
| - identifier: return.unusedType |
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.
what was reported without this ignore rule ?
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.
> ------ ------------------------------------------------------------------------------------------------------------------------------
Line Service/S3Vectors/src/Input/QueryVectorsInput.php
------ ------------------------------------------------------------------------------------------------------------------------------
124 Method AsyncAws\S3Vectors\Input\QueryVectorsInput::getFilter() never returns null so it can be removed from the return type.
🪪 return.unusedType
------ ------------------------------------------------------------------------------------------------------------------------------
So the method never returns null since it uses ?? [], but it's still in the return type.
This pkg doesn't seem to allow changing generated code, so i suppressed it.
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.
This looks like a case where we need to improve the code generator instead.
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.
This looks like a case where we need to improve the code generator instead.
should the code generator skip the null type or not do any null coalescing ?
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.
the getter should not have the ?? []. I guess our code generator has an issue for document shapes (which are quite new, and are not yet used in any merged clients). The @var annotation on the property is also suspicious, with duplicate null types.
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.
this should all be fixed by #2021
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.
thanks, i rebased on your branch, seems to have fixed it
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.
Please rebase again on latest master as my PR is now merged.
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.
done
578e394 to
bf2b90f
Compare
CRUD operations for creating s3 vector buckets, indexes and vectors + query.
bf2b90f to
3823694
Compare
Fixes #2010
Relates to symfony/ai#1251