Skip to content

refactor (ai): remove obsolete handling of temperature when it is undefined#5769

Closed
gr2m wants to merge 9 commits intov5from
v5-5765-remove-temperature-special-handling
Closed

refactor (ai): remove obsolete handling of temperature when it is undefined#5769
gr2m wants to merge 9 commits intov5from
v5-5765-remove-temperature-special-handling

Conversation

@gr2m
Copy link
Copy Markdown
Collaborator

@gr2m gr2m commented Apr 14, 2025

Background

towards #5765

Summary

temperature now defaults to undefined, so the special handling is no longer necessary - at least that's my assumption given that tests pass. Please verify before merging.

Tasks

  • Tests for the changes have been added (for bug fixes / features)
  • n/a Docs have been added / updated (for bug fixes / features)
  • If required, a patch changeset for relevant packages has been added
  • You've run pnpm prettier-fix to fix any formatting issues

@gr2m gr2m mentioned this pull request Apr 14, 2025
8 tasks
Comment thread packages/ai/core/middleware/default-settings-middleware.ts Outdated
@lgrammel
Copy link
Copy Markdown
Collaborator

Please add a changeset.

@lgrammel
Copy link
Copy Markdown
Collaborator

Is that all that is needed to remove the default temperature?

@gr2m
Copy link
Copy Markdown
Collaborator Author

gr2m commented Apr 15, 2025

Please add a changeset.

done: b0ace14

Is that all that is needed to remove the default temperature?

I think so? I added a test for the settings middleware to verify the new behavior and to prevent future regressions: 756e597

Comment thread packages/ai/core/middleware/default-settings-middleware.test.ts Outdated
@lgrammel
Copy link
Copy Markdown
Collaborator

The main setting that defaults temp to 0 is in packages/ai/core/prompt/prepare-call-settings.ts line 101

Comment on lines +44 to +56
it('should default temparature to undefined', async () => {
const middleware = defaultSettingsMiddleware({
settings: {},
});

const result = await middleware.transformParams!({
type: 'generate',
params: BASE_PARAMS,
});

expect(result.temperature).toBeUndefined();
});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we can remove this test, I added another one to packages/ai/core/prompt/prepare-call-settings.test.ts

Suggested change
it('should default temparature to undefined', async () => {
const middleware = defaultSettingsMiddleware({
settings: {},
});
const result = await middleware.transformParams!({
type: 'generate',
params: BASE_PARAMS,
});
expect(result.temperature).toBeUndefined();
});

@gr2m
Copy link
Copy Markdown
Collaborator Author

gr2m commented Apr 16, 2025

The main setting that defaults temp to 0 is in packages/ai/core/prompt/prepare-call-settings.ts line 101

🤦🏼 sorry I missed that. Addressed via 3963586.

Should be ready now. Once merged we can close #5765

@lgrammel
Copy link
Copy Markdown
Collaborator

Before merging, I want to think about it more. The alternative would have been to default to 0 and allow users setting it to null to reset, which may or may not be the better DX.

@lgrammel
Copy link
Copy Markdown
Collaborator

Preferring #5890 because temperature=0 is a great default for many models.

@lgrammel lgrammel closed this Apr 23, 2025
@lgrammel lgrammel deleted the v5-5765-remove-temperature-special-handling branch July 2, 2025 10:52
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.

2 participants