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: 4238 - fix default max_tokens set on remote models #4266

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

louis-jan
Copy link
Contributor

Describe Your Changes

I've resolved the issue where it sets the default context_length as the first value for max_tokens.

CleanShot 2024-12-12 at 13 16 28

Fixes Issues

Changes made

  1. models.json:

    • Moved the max_tokens parameter above the temperature parameter for better grouping in two sections of the file.
  2. ModelDropdown/index.tsx:

    • Updated overriddenParameters:
      • Changed ctx_len to be undefined if isLocalEngine returns false, otherwise use defaultContextLength.
      • Changed max_tokens to use either the model's max_tokens or defaultContextLength based on whether isLocalEngine returns false.
  3. useCreateNewThread.ts:

    • Updated imports to include isLocalEngine.
    • Modified overriddenSettings and overriddenParameters:
      • Changed ctx_len to behave similarly to the changes in ModelDropdown/index.tsx.
      • Changed max_tokens to use either the model's token_limit or defaultContextLength based on whether isLocalEngine returns false.
  4. modelEngine.ts:

    • Made isLocalEngine handle an undefined engine by returning false, ensuring safety against calling the method with a null or undefined value.

@github-actions github-actions bot added the type: bug Something isn't working label Dec 12, 2024
@louis-jan louis-jan requested a review from urmauur December 12, 2024 06:21
Copy link
Contributor

Barecheck - Code coverage report

Total: 68.65%

Your code coverage diff: 0.01% ▴

Uncovered files and lines
FileLines
web/containers/ModelDropdown/index.tsx102, 113, 123-124, 127, 129-130, 132-133, 135-138, 140, 148, 165-167, 169, 171, 190, 194, 203, 210, 213-214, 249-251, 255, 279-281, 283, 287, 290-295, 302-303, 314, 336, 352, 373, 382, 388, 396-397, 400-405, 408, 411, 463-464, 466, 492, 500, 502, 527, 532-533, 535, 539-541, 543-544, 546, 556-559, 587, 595, 597
web/hooks/useCreateNewThread.ts43, 45, 50-51, 54-55

Copy link
Contributor

This is the build for this pull request. You can download it from the Artifacts section here: Build URL.

@louis-jan louis-jan merged commit 707c23f into dev Dec 13, 2024
21 checks passed
@louis-jan louis-jan deleted the fix/4238-wrong-max-tokens-remote-models branch December 13, 2024 05:41
@github-actions github-actions bot added this to the v0.5.12 milestone Dec 13, 2024
@khanakia
Copy link

It is not fixed it's still there in the new version https://github.com/janhq/jan/releases/tag/v0.5.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants