Skip to content

Comments

[Remote Inference] Supported params are ignored#1562

Merged
kaisopos merged 13 commits intomainfrom
kostas/remote_infer_unsupported_params
Mar 28, 2025
Merged

[Remote Inference] Supported params are ignored#1562
kaisopos merged 13 commits intomainfrom
kostas/remote_infer_unsupported_params

Conversation

@kaisopos
Copy link
Contributor

@kaisopos kaisopos commented Mar 21, 2025

Description

Updates:

  1. Validating that all params are supported, before the remote API call.
  2. Adding 2 missing params (min_p, stop_token_ids) in _convert_conversation_to_api_input (base remote engine)
  3. Open AI: Dropping logit_bias for "o1-preview" (this model was not working before)
  4. Instead of always using the model params that were used when initializing the inference engine (self._model_params), this change allows users to update the model with a new inference config (inference_config.model) after the engine is instantiated. This allows users to use the same OpenAI engine for gpt-4, but later (without re-instantiating the engine) they can pass in a new config and run inference for OpenAI's o1-preview.

Related issues

Fixes OPE-1123

Before submitting

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

Reviewers

At least one review from a member of oumi-ai/oumi-staff is required.

@kaisopos kaisopos marked this pull request as draft March 22, 2025 00:16
@wizeng23 wizeng23 requested review from nikg7, taenin and wizeng23 and removed request for nikg7, taenin and wizeng23 March 25, 2025 05:44
@kaisopos kaisopos changed the title [Remote Inference] Bug: Supported params are ignored [WIP][Remote Inference] Bug: Supported params are ignored Mar 25, 2025
@kaisopos kaisopos changed the title [WIP][Remote Inference] Bug: Supported params are ignored [Remote Inference] Supported params are ignored Mar 26, 2025
@kaisopos kaisopos requested a review from oelachqar March 26, 2025 00:03
@kaisopos kaisopos marked this pull request as ready for review March 26, 2025 00:11
@kaisopos kaisopos requested a review from optas March 26, 2025 00:11
@kaisopos kaisopos changed the title [Remote Inference] Supported params are ignored [WIP][Remote Inference] Supported params are ignored Mar 26, 2025
@kaisopos kaisopos marked this pull request as draft March 26, 2025 00:30
"stop_strings",
"stop_token_ids",
"min_p",
"guided_decoding",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: min_p and stop_token_ids were newly added here

@kaisopos kaisopos changed the title [WIP][Remote Inference] Supported params are ignored [Remote Inference] Supported params are ignored Mar 26, 2025
@kaisopos kaisopos marked this pull request as ready for review March 26, 2025 01:16
return "OPENAI_API_KEY"

@override
def _convert_conversation_to_api_input(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function could be omitted, since logit_bias is empty by default & the super function has been updated to NOT include logit_bias in the API call if empty. This fn will only protect the user if they unintentionally set this. It is questionable though if it's a better experience to silently ignore this or just fail.

@kaisopos kaisopos marked this pull request as draft March 26, 2025 20:53
@kaisopos kaisopos marked this pull request as ready for review March 26, 2025 22:41
@kaisopos kaisopos requested review from taenin and wizeng23 March 27, 2025 17:58
@kaisopos kaisopos merged commit b66ff54 into main Mar 28, 2025
2 checks passed
@kaisopos kaisopos deleted the kostas/remote_infer_unsupported_params branch March 28, 2025 20:33
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