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

Saturday Cleanup: Improving Code Quality / Typing #4206

Closed
10 of 15 tasks
krrishdholakia opened this issue Jun 15, 2024 · 4 comments
Closed
10 of 15 tasks

Saturday Cleanup: Improving Code Quality / Typing #4206

krrishdholakia opened this issue Jun 15, 2024 · 4 comments

Comments

@krrishdholakia
Copy link
Contributor

krrishdholakia commented Jun 15, 2024

Creating a list to track improvements to code quality.

Goals:

  • Improve user experience with client -> add typing to returned functions (.completions, etc.)
  • Reduce errors -> by adding typing to common utilities
  • Make it easier to debug errors
  • Better Security practices

Actions:

  • Set file limit to 10k lines
  • Use isort for sorting imports
  • fix model response object typing for stream vs. non-stream
  • add overrides for completion/.acompletion calls
  • Fix loader-utils in the docs security issue you raised
  • Don't include docs / misc files in litellm docker fix(build): .dockerignore not picked up #3116

Ideas:

  • have router debugging work with litellm.set_verbose=True fixed by moving to logging library
  • refactor Logging into a separate file
  • use typing with overloads for .completion + .acompletion calls
  • Typing to the Logging: class
  • The code is not linted or annotated.
  • utils.py seems to be getting out of hand with the amount of code that's in a single file and should be split up.
  • Exception handling has stuff like bare "except:" or "except Exception:" all over the place rather than catching specific exceptions, which can make debugging difficult, and it may unexpectedly silence exceptions you don't want to be silenced.
  • Uses a combination of loggers and prints without a particular structure, lots of commented out statements, etc.
  • Improve our JWT signature implementation on Admin UI: [Bug]: The JWT encode implementation uses a weak static signing secret of "secret"  #2158
@krrishdholakia krrishdholakia pinned this issue Jun 15, 2024
@krrishdholakia krrishdholakia changed the title Improving Code Quality / Typing / Cleanup Saturday Cleanup: Improving Code Quality / Typing / Cleanup Jun 15, 2024
@krrishdholakia krrishdholakia changed the title Saturday Cleanup: Improving Code Quality / Typing / Cleanup Saturday Cleanup: Improving Code Quality / Typing Jun 15, 2024
@ishaan-jaff
Copy link
Contributor

ishaan-jaff commented Jun 15, 2024

Maybe there is some linting but overall the code still looks messy visually. Looking at utils.py, the imports are not sorted, there are no line length constraints, functions could benefit from being split up (e.g. success_handler), etc.
I only played around briefly with it but had some issues with the lunary & helicone callbacks. Specifically, the lunary one got fixed here (#3424) after I emailed lunary. I was looking at the handlers also and noticed e.g. lunary reassigns kwargs = self.model_call_details and uses the messages from there, but in helicone the messages are retrieved from a different kwargs. Overall a confusing experience and not fun to debug in such a big file.
Tightening linting, removing bare excepts, unifying logging (we like structlog), splitting files up would be good first steps.

@krrishdholakia
Copy link
Contributor Author

krrishdholakia commented Jun 15, 2024

imports are not sorted

  • use isort

there are no line length constraints

  • have black formatting enforce line length constraints

functions could benefit from being split up (e.g. success_handler)

  • remove unused functions

unifying logging

  • move to using an environment variable LITELLM_LOG
  • move to just using the logging library instead of print_verbose

splitting files up

  • implement a file length limit (is this possible)?

@krrishdholakia
Copy link
Contributor Author

krrishdholakia commented Jun 16, 2024

Results:

  • Logging class is now refactored to a separate file
  • utils.py + proxy_server.py are both <10k lines of code
  • file length limits are now enforced (max 10k lines)
  • isort is now implemeted
  • logging v/s print_verbose -> moving to using logging library consistently. Deprecation warning added for moving from litellm.set_verbose=True to os.environ["LITELLM_LOG"]. This is already implemented for proxy. Bringing logic to sdk.
  • New ModelResponseChunk to fix the linting errors caused by using ModelResponse object for both streaming and non-streaming responses

@krrishdholakia
Copy link
Contributor Author

krrishdholakia commented Jun 16, 2024

We should do this again next Saturday (06/22).

Next week items:

  • add overrides for completion/.acompletion calls
  • Typing to the Logging: class
  • Improve our JWT signature implementation on Admin UI
  • update docs on easily debugging proxy with env var

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

No branches or pull requests

2 participants