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.

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

fix: Supported model names #133

Merged
merged 9 commits into from
Sep 17, 2024
Merged

fix: Supported model names #133

merged 9 commits into from
Sep 17, 2024

Conversation

ZhongpinWang
Copy link
Contributor

@ZhongpinWang ZhongpinWang commented Sep 16, 2024

Context

AI/gen-ai-hub-sdk-js-backlog#113.

Model name autocompletion

  • for openai in foundation-models
  • for orchestration

Model types are defined centrally in core and then separately imported into these two packages.

Definition of Done

  • Code is tested (Unit, E2E)
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • (Optional) Aligned changes with the Java SDK
  • (Optional) Release notes updated

@ZhongpinWang ZhongpinWang requested review from deekshas8, marikaner and MatKuhr and removed request for deekshas8 September 16, 2024 10:38
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

Maybe we can define the constants only once and use the union of LLM Access + orchestration. And then use Omit<.> within the specific clients?

While there will be exceptions, probably most models will be the same across both, so we would only have to omit a few models, instead of duplicating a lot..

@ZhongpinWang
Copy link
Contributor Author

@MatKuhr @deekshas8 Thanks for your feedback! Still a bit hesitating if excluding models would be a good idea as per default, as per default, adding a new model in a reused model type file and then exclude is a bit error-prone as we might simply forget to exclude it. But anyways since it is now quite simple to do so, I weight reusability a bit higher than maintainability, and refactored accordingly.

@MatKuhr
Copy link
Member

MatKuhr commented Sep 16, 2024

we might simply forget to exclude it.

Equally, we might forget to add a model in two places, i.e. by accident only adding it in one. For now we can assume 9/10 models will be available in both LLM access & orchestration, so I'd rather take the chance on exclusion 😉

packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/model-types.ts Outdated Show resolved Hide resolved
packages/orchestration/src/model-types.ts Outdated Show resolved Hide resolved
packages/core/src/model-types.ts Outdated Show resolved Hide resolved
packages/foundation-models/src/openai/model-types.ts Outdated Show resolved Hide resolved
packages/core/src/model-types.ts Show resolved Hide resolved
Copy link
Contributor

@deekshas8 deekshas8 left a comment

Choose a reason for hiding this comment

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

LGTM

@ZhongpinWang ZhongpinWang merged commit 4068a88 into main Sep 17, 2024
9 checks passed
@ZhongpinWang ZhongpinWang deleted the fix-supported-model-names branch September 17, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants