support modular models in multiclient #2556

kazrael2119 · 2024-06-04T07:35:14Z

fixes #2554
fixes #2611

see design doc.

Discussion Conclusion:

Model Generation:

Decision: All models will be generated under the models/ directory.
Rationale: This standardizes the location for model files, making it easier to manage and locate them.

Name Collision Detection:

Decision: Implement a mechanism to detect name collisions among the models.
Suffix Strategy: In cases of name collisions, suffix the model names with _N where N is a unique number.
Rationale: This approach ensures unique model names, preventing conflicts during generation.

Handling Name Collisions (Process):

Action Required: Library authors should resolve name conflicts identified by the _N suffix during code review or API review.
Resolution Methods:
- Use the @clientName attribute to specify unique names.
- Update the specification to avoid name collisions.

Warning and Reporting:

Emitter Output: The emitter should generate a warning listing the models with name collisions.
Actionable Steps: Provide actionable steps for resolving these collisions, such as reviewing and updating model names or specifications.

Action Items

implement single model subpath export.
detect name collision
handle name collision
report diagnostic warning
add document on how to resolve the warning

qiaozha

I think this is a good candidate for smoke test as it has serialize utils for multi sub client clients in Modular. @joheredi Let me know if you have any concerns.

qiaozha · 2024-06-05T15:03:07Z

...ec-test/test/ai_modelClient/generated/typespec-ts/src/chatCompletions/utils/serializeUtil.ts

+}
+
+/** serialize function for ChatRequestMessageUnion */
+export function serializeChatRequestMessageUnion(


@joheredi, Currently getAllModels are not bounded by the client decorators, so even if we have generate the models/models.ts in several different subclient's folder, they are actually redundant, and serializeUtils are bound with the models, in this fix, they are also redundant https://github.com/Azure/autorest.typescript/pull/2556/files#diff-182a11512248c84c2b807878be3b3743021b2565342194c7b77d269fefdcba59

I vaguely remember we have some discussion about this, when we support multiclient in modular, the conclusion is that we should make sure to have subpath export for both api and models for each subclient.

packages/typespec-ts/src/modular/buildOperations.ts

packages/typespec-test/test/ai_modelClient/generated/typespec-ts/package.json

...est/test/NetworkAnalytics.Management/generated/typespec-ts/src/classic/dataProducts/index.ts

qiaozha · 2024-07-16T07:06:58Z

Summary of changes in this PR:

move models.ts and pagingTypes.ts from subpath export ./subClient/models to the top level ./models
move ./subClient/models/options.ts and ./models/options.ts to api layer as operation option is closely tied to operations.
re-export all models in the subclient classical layer.
remove the export of XXXClientOptions in classical client. instead of export it from the api layer because we would need to re-export all the operation options to the top level anyway.
add diagnostic of "duplicate-model-name" and report it during model name conflict detection.
rename rlc option enableModelNamespace into autoResolveModelConflict and changing the rename strategy from add namespace prefix into add number N suffix. And because we have normalize name logic for model name _N suffix will be normalized into N suffix, it would be risky to just remove normalization in order to keep _N suffix, as we discussed before.

MaryGao · 2024-07-18T13:08:43Z

packages/typespec-test/test/ai_modelClient/generated/typespec-ts/.tshy/browser.json

+  ],
+  "exclude": [],
+  "compilerOptions": {
+    "outDir": "../.tshy-build/browser"


Should remove files under .tshy?

MaryGao · 2024-07-18T13:30:52Z

packages/typespec-test/test/anomalyDetector/generated/typespec-ts/src/rest/outputModels.ts

@@ -2,7 +2,7 @@
 // Licensed under the MIT license.

 /** The response of entire anomaly detection. */
-export interface UnivariateUnivariateEntireDetectionResultOutput {
+export interface UnivariateEntireDetectionResultOutput {


It seems we have name collisions here(that's why we have prefix with namespace before), but i didn't notice any name suffix with _N, could you share the link with me?

MaryGao · 2024-07-18T13:49:42Z

...s/typespec-test/test/hierarchy_generic/generated/typespec-ts/review/hierarchy-generic.api.md

@@ -9,31 +9,31 @@ import { OperationOptions } from '@azure-rest/core-client';
 import { Pipeline } from '@azure/core-rest-pipeline';

 // @public (undocumented)
-export interface A {
+export interface A0 {


haha, we start with _0? i was wondering if we just keep A as A for the first one?

MaryGao · 2024-07-18T13:59:14Z

packages/typespec-test/test/openai_modular/generated/typespec-ts/review/openai_modular.api.md

@@ -465,6 +459,7 @@ export interface ChatRequestUserMessage extends ChatRequestMessage {
 // @public
 export interface ChatResponseMessage {
    content: string | null;
+    // Warning: (ae-forgotten-export) The symbol "AzureChatExtensionsMessageContext" needs to be exported by the entry point index.d.ts


Could fix this warning?

MaryGao · 2024-07-18T13:59:51Z

packages/typespec-test/test/openai_modular/generated/typespec-ts/review/openai_modular.api.md

@@ -129,12 +129,6 @@ export interface AzureChatExtensionDataSourceResponseCitation {
    url?: string;
 }



Is this expected? it seems relevant to this change: #2556 (comment).

MaryGao · 2024-07-18T14:04:17Z

packages/typespec-ts/src/utils/namespaceUtils.ts

+      nameSet.set(model.name, [
+        ...nameSet.get(model.name)!,
+        model.__raw! as Model
+      ]);


nit: could we directly add items into original array? so that we don't need to build a new array every time.

MaryGao · 2024-07-18T14:15:37Z

packages/typespec-ts/src/modular/buildSerializeUtils.ts

@@ -19,6 +19,7 @@ import {
  serializeRequestValue
 } from "./helpers/operationHelpers.js";
 import { ModularCodeModel, Type } from "./modularCodeModel.js";
+import path from "path/posix";


Why path/posix not path directly? it seems posix would stick to posix style, not sure if we have issue in non-unix environement https://nodejs.org/api/path.html#windows-vs-posix?

MaryGao · 2024-07-18T14:21:37Z

packages/typespec-ts/src/utils/interfaces.ts

 import { KnownMediaType } from "./mediaTypes.js";

 export interface SdkContext extends TCGCSdkContext {
  rlcOptions?: RLCOptions;
  generationPathDetail?: GenerationDirDetail;
  hasApiVersionInClient?: boolean;
+  nameModelMap?: Map<string, Model[]>;


you know we have a ContextManager, i was wondering if saving this information into there would be more proper?

MaryGao · 2024-07-18T14:23:11Z

packages/typespec-ts/src/transform/transfromRLCOptions.ts

@@ -61,7 +61,7 @@ function extractRLCOptions(
    dpgContext,
    emitterOptions
  );
-  const enableModelNamespace = getEnableModelNamespace(
+  const autoResolveModelConflict = getEnableModelNamespace(


Should we change the getter function name here also?

MaryGao · 2024-07-18T14:24:20Z

packages/typespec-ts/src/utils/namespaceUtils.ts

@@ -64,15 +67,37 @@ export function getOperationNamespaceInterfaceName(

 export function detectModelConflicts(dpgContext: SdkContext) {


Do we leverage this in RLC also? Is it problematic when we plan to get rid of tcgc dependency in RLC?

qiaozha · 2024-10-09T08:19:59Z

close it as most of the change has been taken by binder pr.

kazrael2119 and others added 4 commits June 4, 2024 15:33

add ai modelClient smoke test

b055a22

fix serialize util subpath

ec24310

Update emitUtil.ts

5d406ef

update

ce95197

qiaozha marked this pull request as ready for review June 5, 2024 06:50

qiaozha requested review from sarangan12, joheredi, qiaozha and MaryGao as code owners June 5, 2024 06:50

qiaozha approved these changes Jun 5, 2024

View reviewed changes

qiaozha reviewed Jun 5, 2024

View reviewed changes

qiaozha mentioned this pull request Jun 23, 2024

revisit modular models generation in multi clients #2611

Open

qiaozha added 3 commits July 2, 2024 14:37

merge main and resolve conflicts

902e7ee

regen

92ef3a4

merge main

698f106

qiaozha self-assigned this Jul 3, 2024

qiaozha added 2 commits July 3, 2024 17:33

fix ai model client

ab3e556

fix models import issues

8ae6bd7

qiaozha reviewed Jul 3, 2024

View reviewed changes

packages/typespec-ts/src/modular/buildOperations.ts Outdated Show resolved Hide resolved

qiaozha and others added 2 commits July 3, 2024 19:33

Update packages/typespec-ts/src/modular/buildOperations.ts

2655034

fix bug

4516ff9

qiaozha reviewed Jul 3, 2024

View reviewed changes

packages/typespec-test/test/ai_modelClient/generated/typespec-ts/package.json Outdated Show resolved Hide resolved

qiaozha added 2 commits July 4, 2024 10:17

fix loadtesting multi client

a627119

remove models in subclient subpath export

19e2183

qiaozha changed the title ~~add ai modelClient smoke test~~ support modular models in multiclient Jul 4, 2024

qiaozha added 5 commits July 5, 2024 14:24

merge main

6d9d4ef

reserve work

67a0081

fix model exports

2be3c51

Merge branch 'main' into pr/kazrael2119/2556

3bcf088

regen-ai-model-client

600b37a

qiaozha added 2 commits July 15, 2024 17:32

reserve work

00ab482

resolve model name conflicts

8192e71

MaryGao reviewed Jul 15, 2024

View reviewed changes

...est/test/NetworkAnalytics.Management/generated/typespec-ts/src/classic/dataProducts/index.ts Show resolved Hide resolved

qiaozha marked this pull request as draft July 16, 2024 02:58

qiaozha added 3 commits July 16, 2024 11:34

fix ci

d2aa06d

merge main

51f6daf

fix ci

da96ea1

qiaozha marked this pull request as ready for review July 16, 2024 07:07

qiaozha added 2 commits July 17, 2024 19:45

merge main and resolve conflicts

52c2fcb

resolve conflicts

7e2f0db

MaryGao reviewed Jul 18, 2024

View reviewed changes

qiaozha added 6 commits August 14, 2024 14:15

merge main

57fb404

resolve conflicts

3932b33

fix smoke test

eca314e

merge main and resolve conflicts

d785c18

merge main and resolve conflicts

745e16b

try to use binder helper

3972925

qiaozha mentioned this pull request Aug 20, 2024

Binder models tcgc #2759

Open

qiaozha closed this Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support modular models in multiclient #2556

support modular models in multiclient #2556

kazrael2119 commented Jun 4, 2024 •

edited by MaryGao

Loading

qiaozha left a comment

qiaozha Jun 5, 2024

qiaozha commented Jul 16, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

MaryGao Jul 18, 2024

qiaozha commented Oct 9, 2024

		@@ -129,12 +129,6 @@ export interface AzureChatExtensionDataSourceResponseCitation {
		url?: string;
		}

		@@ -64,15 +67,37 @@ export function getOperationNamespaceInterfaceName(

		export function detectModelConflicts(dpgContext: SdkContext) {

support modular models in multiclient #2556

support modular models in multiclient #2556

Conversation

kazrael2119 commented Jun 4, 2024 • edited by MaryGao Loading

Discussion Conclusion:

Model Generation:

Name Collision Detection:

Handling Name Collisions (Process):

Warning and Reporting:

Action Items

qiaozha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiaozha commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiaozha commented Oct 9, 2024

kazrael2119 commented Jun 4, 2024 •

edited by MaryGao

Loading