Skip to content

Improvements on typing #279

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
Merged
merged 1 commit into from
Apr 1, 2025
Merged

Improvements on typing #279

merged 1 commit into from
Apr 1, 2025

Conversation

tsmith023
Copy link
Collaborator

  • Fixes bad typing of GenerativeConfigRuntime
  • adds mocks for this usage
  • renames generativeConfigRuntime to generativeParameters to match Python naming

@tsmith023
Improvements:
af4f7c1 
- Fixes bad typing of `GenerativeConfigRuntime`
- adds mocks for this usage
- renames `generativeConfigRuntime` to `generativeParameters` to match Python naming
@tsmith023 tsmith023 requested a review from bevzzz April 1, 2025 10:19
orca-security-eu[bot]
orca-security-eu bot reviewed Apr 1, 2025
View reviewed changes
Copy link

@orca-security-eu orca-security-eu bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

bevzzz
bevzzz approved these changes Apr 1, 2025
View reviewed changes
src/collections/types/generate.ts
@@ -122,18 +122,18 @@ export type GroupedTask<T> = {
type omitFields = 'images' | 'imageProperties';

export type GenerativeConfigRuntime =
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we renamed generativeConfigRuntime to generativeParameters would it make sense to also rename the type? Or was it introduced before dev/1.30?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name of the GenerativeConfigRuntime type here is already aligned with the python client naming of the returned class as _GenerativeConfigRuntime. That renaming to generativeParameters was necessary since it was an inconsistency between the two clients 😁

src/collections/types/generate.ts
Comment on lines +129 to +136
| ModuleConfig<'generative-cohere', GenerativeConfigRuntimeType<'generative-cohere'> | undefined>
| ModuleConfig<'generative-databricks', GenerativeConfigRuntimeType<'generative-databricks'> | undefined>
| ModuleConfig<'generative-dummy', GenerativeConfigRuntimeType<'generative-dummy'> | undefined>
| ModuleConfig<'generative-friendliai', GenerativeConfigRuntimeType<'generative-friendliai'> | undefined>
| ModuleConfig<'generative-google', GenerativeConfigRuntimeType<'generative-google'> | undefined>
| ModuleConfig<'generative-mistral', GenerativeConfigRuntimeType<'generative-mistral'> | undefined>
| ModuleConfig<'generative-nvidia', GenerativeConfigRuntimeType<'generative-nvidia'> | undefined>
| ModuleConfig<'generative-ollama', GenerativeConfigRuntimeType<'generative-ollama'> | undefined>
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: make ModuleConfig.config optional (config?: C), then have individual ModuleConfig variants make it required explicitly?

typescript-client/src/collections/config/types/index.ts

Lines 17 to 20 in f3ab9f8

export type ModuleConfig<N, C = undefined> = {
name: N;
config: C;
};

Idk, maybe it's less flexible, but I just noticed the repeated | undefined and thought about this.

Example TS Playground

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one's because of annoying aspect of the design decision here that I want the type C to be inferred as undefined if it is. If you set config?: C then doing config: undefined won't infer C as undefined but instead as unknown. Agreed that the | undefined is cumbersome here but I think it's necessary because of the above

@tsmith023 tsmith023 merged commit 184f8d6 into dev/1.30 Apr 1, 2025
11 of 13 checks passed
@tsmith023 tsmith023 deleted the 1.30/fix-inconsistent-naming-types-in-generative-config-runtime branch April 1, 2025 11:42
tsmith023 added a commit that referenced this pull request Apr 3, 2025
@tsmith023 @bevzzz
Dev/1.30 (#275)
8f82eac 
* Add support for defining generative config at query-time (dynamic RAG)

* Fix unit test

* Again fix unit test

* Add concurrency limit to CI on a per branch basis to cancel old runs

* Add test of string-only usage with runtime generative

* Add factory to produce user friendly gen runtime config objects

* Remove `it.only` from test

* Update CI images

* Update name of version checker method

* Update to use latest proto with optional model fix in openai

* Fix unit test

* Update CI images

* merge

* Add ability to define tenant-level perms for `data` and `tenants` perms

* Make the next publish job push a next tag (add bash script to do this automatically in future)

* 3.5.0-beta.0

* Fix action typo, add generic to catch this in future

* 3.5.0-beta.1

* Fix broken add/remove permissions method

* 3.5.0-beta.2

* Make fixes in response to user feedback

* Improvements: (#279)

- Fixes bad typing of `GenerativeConfigRuntime`
- adds mocks for this usage
- renames `generativeConfigRuntime` to `generativeParameters` to match Python naming

* Use REST instead of gRPC for `tenants.getByName` when available due to filtering issues with rbac

* Add back tests of unsupported error throwing for <1.25

* chore: update OpenAPI schema

* feat: enable dynamic user management, add db/oidc namespaces

* refactor: re-use common code snippets

* test: extend integration test suite

* refactor(test): use concise Promise.all

* feat(breaking): include user types in user assignments

* chore: format and lint

* test: activate test case for 'oidc' users

* chore: lint and format

* refactor: replace .reduce with .map where possible

Rewrote other .reduce occurences in a more succinct manner

* test: use valid tenant name

* refactor: collect optional parameters in an object

* test: add test case w/ includePermissions=true

* chore: lint and format

* Add missing modules and module params for
- `baseURL` in `generative-anthropic`
- `videoIntervalSeconds` in `multi2vec-google`
- `outputEncoding` in `Multi2VecVoyageAIConfig`
- the entire `text2vec-nvidia` module

* fix: expect no response for /activate and /deactivate

* fix: allow expected code only for instances of WeaviateUnexpectedStatusCodeError

* refactor: declare types in **/types.ts

* test: await on all expectations which should resolve/reject

* refactor: replace postNoBody with a more explicit postReturn<null, R>

* chore: add documentation for dynamic user management

* chore: lint and format

* test: use different port binding for backup/unit.test.ts

Seems like Node 18 does not automatically 'resolve'
port collisions, so 2 test running Express servers
on the same ports will interfere with one another.

* feat: add revokeKey option to /deactivate

* Remove `--tag next` from CI publish in preparation for merging

---------

Co-authored-by: dyma solovei <dyma@weaviate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orca-security-eu orca-security-eu[bot] orca-security-eu[bot] left review comments

@bevzzz bevzzz bevzzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsmith023 @bevzzz