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

Changes model_id path parameter to inference_id in openAI and Cohere notebooks #214

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

szabosteve
Copy link
Contributor

Overview

This PR renames the model_id path parameter to inference_id in the OpenAI and the Cohere notebooks.

@szabosteve szabosteve added the documentation Improvements or additions to documentation label Mar 25, 2024
Copy link

gitnotebooks bot commented Mar 25, 2024

Found 2 changed notebooks. Review the changes at https://gitnotebooks.com/elastic/elasticsearch-labs/pull/214

@szabosteve szabosteve self-assigned this Mar 25, 2024
@miguelgrinberg
Copy link
Collaborator

I do not have the history on why this argument was renamed, but I assume people working with 8.12 will see errors if they use the new name? In that case, should we note that model_id should be used with old versions?

@szabosteve szabosteve marked this pull request as ready for review March 26, 2024 08:20
@miguelgrinberg
Copy link
Collaborator

@szabosteve there is a model_id in line 247 of the cohere notebook. Does that also need to be changed to inference_id?

@szabosteve
Copy link
Contributor Author

@miguelgrinberg No, it doesn't need to be changed. Unfortunately, the inference pipeline processor still uses model_id. There is a plan to change it in the 8.14 timeframe.

@szabosteve szabosteve merged commit d1eb753 into main Mar 26, 2024
5 checks passed
@szabosteve szabosteve deleted the szabosteve/rename-model-id branch March 26, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants