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.

Sign up for GitHub

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

Jump to bottom

[Search] Regenerate with 2024-05-01-preview spec #29594

Merged
merged 1 commit into from May 20, 2024
Merged

[Search] Regenerate with 2024-05-01-preview spec #29594

merged 1 commit into from May 20, 2024

Conversation

dgetu
Copy link
Member

@dgetu dgetu commented May 7, 2024 •
edited

Packages impacted by this PR

@azure/search-documents

Describe the problem that is addressed by this PR

Adds 2024 May preview features.

Designed to be reviewed on a per-commit basis.

Depends on #29595

Are there test cases added in this PR? (If not, why?)

Tests are currently failing locally.

Checklists

  • Added impacted package name to the issue description
  • Added a changelog (if necessary)

@github-actions github-actions bot added the Search label May 7, 2024
@dgetu dgetu force-pushed the search/2024-05-01-preview branch 6 times, most recently from 685b434 to 9404701 Compare May 7, 2024 07:04
@dgetu dgetu marked this pull request as ready for review May 7, 2024 07:04
@dgetu dgetu force-pushed the search/2024-05-01-preview branch from 9404701 to 274bcba Compare May 7, 2024 09:37
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/search-documents

@dgetu dgetu force-pushed the search/2024-05-01-preview branch from 274bcba to 309d00e Compare May 8, 2024 19:33
@dgetu dgetu requested a review from maorleger as a code owner May 8, 2024 19:33
@dgetu dgetu force-pushed the search/2024-05-01-preview branch 6 times, most recently from dd2d3e6 to 20de322 Compare May 9, 2024 23:16
maorleger
maorleger reviewed May 10, 2024
View reviewed changes
Copy link
Member

@maorleger maorleger left a comment

Choose a reason for hiding this comment

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

Havent finished reviewing yet but figured I'll share whatever comments I spotted already. Will continue after my morning meeting 👍

.vscode/cspell.json Outdated Show resolved Hide resolved
sdk/search/search-documents/CHANGELOG.md

### Breaking Changes

- Fixed an incorrect enum variant in `KnownVectorQueryKind` [#29601](https://github.com/Azure/azure-sdk-for-js/pull/29601)

### Features Added
Copy link
Member

Choose a reason for hiding this comment

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

😍

sdk/search/search-documents/review/search-documents.api.md
@@ -15,6 +15,23 @@ import { Pipeline } from '@azure/core-rest-pipeline';
import { RestError } from '@azure/core-rest-pipeline';
import { TokenCredential } from '@azure/core-auth';

// @public
export interface AIServicesVisionParameters {
apiKey?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell me more about this? I see both apiKey and authIdentity here and I wonder what scenarios do we have where both are provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that exactly one of these should be set - one uses a key credential where the other uses managed identity. There's an authored convenience pattern elsewhere in this APIView where a discriminated union is used to enforce such constraints at compile time. This is how it's generated, with every auth kind exposed as optional properties. I've opted to maintain consistency with the rest of the package here, and only implement the convenience with AMLParameters because it's called out explicitly in the documentation there.

sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
sdk/search/search-documents/review/search-documents.api.md Outdated Show resolved Hide resolved
@dgetu dgetu force-pushed the search/2024-05-01-preview branch from 20de322 to 590d5a8 Compare May 10, 2024 21:12
@dgetu dgetu requested a review from maorleger May 10, 2024 21:15
@dgetu dgetu force-pushed the search/2024-05-01-preview branch from 590d5a8 to e7c2614 Compare May 10, 2024 21:18
@dgetu dgetu force-pushed the search/2024-05-01-preview branch 4 times, most recently from 7e290aa to ca94b57 Compare May 16, 2024 18:41
maorleger
maorleger reviewed May 16, 2024
View reviewed changes
sdk/search/search-documents/src/indexModels.ts
@@ -994,3 +1027,26 @@ export interface VectorSearchOptions<TModel extends object> {
*/
filterMode?: VectorFilterMode;
}
/** The threshold used for vector queries. */
export interface BaseVectorThreshold {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this base is needed or does anything, does it? If you remove it (and the two interfaces below no longer extend this) does anything change or break?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a rename of a generated type. I'll open an issue to update all such types to be emitted as Autorest transforms.

maorleger
maorleger reviewed May 17, 2024
View reviewed changes
sdk/search/search-documents/review/search-documents.api.md
export interface KeyAuthAzureMachineLearningVectorizerParameters extends BaseAzureMachineLearningVectorizerParameters {
authenticationKey: string;
// (undocumented)
authKind: "key";
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Maor will chat with other architects about using classes instead of discriminated unions (not blocking)

maorleger
maorleger reviewed May 17, 2024
View reviewed changes
sdk/search/search-documents/src/searchClient.ts
if (!vectorQuery) {
return vectorQuery;
}
private convertVectorQuery<T extends VectorQuery<TModel>>(vectorQuery: T): GeneratedVectorQuery {
Copy link
Member

Choose a reason for hiding this comment

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

TODO: toGenerated / fromGenerated naming conventions + this can be a function. Not blocking

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I'll open an issue tracking migrating all such functions to their own file/namespace as well.

@dgetu dgetu force-pushed the search/2024-05-01-preview branch from ca94b57 to 1f1271a Compare May 20, 2024 17:56
@dgetu dgetu force-pushed the search/2024-05-01-preview branch from 1f1271a to f51681a Compare May 20, 2024 19:51
@dgetu
Copy link
Member Author

dgetu commented May 20, 2024

/check-enforcer override

@dgetu dgetu enabled auto-merge (squash) May 20, 2024 20:02
@dgetu dgetu merged commit 9bacb64 into Azure:main May 20, 2024
8 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maorleger maorleger maorleger approved these changes

@bterlson bterlson Awaiting requested review from bterlson bterlson is a code owner

@xirzec xirzec Awaiting requested review from xirzec xirzec is a code owner

@jeremymeng jeremymeng Awaiting requested review from jeremymeng jeremymeng is a code owner

@mpodwysocki mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki is a code owner

@mikeharder mikeharder Awaiting requested review from mikeharder mikeharder is a code owner

@ckairen ckairen Awaiting requested review from ckairen ckairen is a code owner

@deyaaeldeen deyaaeldeen Awaiting requested review from deyaaeldeen deyaaeldeen is a code owner

Assignees
No one assigned
Labels
Search
Projects
Azure SDK for Search
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dgetu @azure-sdk @maorleger