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: max_tokens in O1 #17703

Merged
merged 2 commits into from
Feb 3, 2025
Merged

fix: max_tokens in O1 #17703

merged 2 commits into from
Feb 3, 2025

Conversation

ex0ns
Copy link
Contributor

@ex0ns ex0ns commented Feb 3, 2025

Description

This bug has been introduced in 89396ae and is assuming that a max_tokens exists for O1 models, which is not the case, failing with a KeyError.

Stacktrace:

  File "/layers/google.python.pip/pip/lib/python3.12/site-packages/tenacity/__init__.py", line 398, in <lambda>
    self._add_action_func(lambda rs: rs.outcome.result())
                                     ^^^^^^^^^^^^^^^^^^^
  File "/layers/google.python.runtime/python/lib/python3.12/concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/layers/google.python.runtime/python/lib/python3.12/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/layers/google.python.pip/pip/lib/python3.12/site-packages/tenacity/asyncio/__init__.py", line 114, in __call__
    result = await fn(*args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/layers/google.python.pip/pip/lib/python3.12/site-packages/llama_index/llms/openai/base.py", line 700, in _achat
    messages=message_dicts, stream=False, **self._get_model_kwargs(**kwargs)
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/layers/google.python.pip/pip/lib/python3.12/site-packages/llama_index/llms/openai/base.py", line 427, in _get_model_kwargs
    if self.model in O1_MODELS and base_kwargs["max_tokens"] is not None:
                                   ~~~~~~~~~~~^^^^^^^^^^^^^^
KeyError: 'max_tokens'

We could add a test for _get_model_kwargs but there are no tests for private method in this package.

Version Bump?

Did I bump the version in the pyproject.toml file of the package I am updating? (Except for the llama-index-core package)

  • Yes
  • No

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.

  • I added new unit tests to cover this change
  • I believe this change is already covered by existing unit tests

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 3, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 3, 2025
@logan-markewich logan-markewich merged commit 8a3feea into run-llama:main Feb 3, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants