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

Loguru logging standardization for LLM Compressor #11

Merged
merged 7 commits into from
Jul 1, 2024
Merged

Conversation

markurtz
Copy link
Collaborator

@markurtz markurtz commented Jun 28, 2024

Enable logging across LLM compressor and enable environment configuration overrides for library imports

SUMMARY:

  • Added LogerConfig and refactored logging implementation to enable environment and configuration overrides for libraries that import LLM compressor.
  • Enhanced docstrings and comments for clarity for logging implementations
  • Refactored across the repo to remove _LOGGER and logging imports to move into loguru pathways.
  • Added unit tests for logger functionality.

TEST PLAN:
Automation tests added, needs further manual testing for verfiication

@markurtz markurtz requested review from Satrat and bfineran June 28, 2024 15:04
@markurtz markurtz self-assigned this Jun 28, 2024
src/llmcompressor/transformers/finetune/text_generation.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/finetune/text_generation.py Outdated Show resolved Hide resolved
tests/unit/test_logger.py Outdated Show resolved Hide resolved
tests/unit/test_logger.py Outdated Show resolved Hide resolved
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Is loguru a lightweight dep?

I ask because vLLM does all their logging through the native python library, so I think it would be nice to match up: https://github.com/vllm-project/vllm/blob/b90d8cd832669b9ad7c48cd9a431e80836778b56/vllm/logger.py

However if there is engineering value in a new logging library I can see the value, especially since it seems to preserve the same interface for creating new logs.

@Satrat
Copy link
Contributor

Satrat commented Jun 28, 2024

@markurtz Got the tests in a passing state. Ran some manual tests of oneshot and reloading, the logs look good in both. Just had a few notes for you above

@Satrat Satrat merged commit 2d2a16a into main Jul 1, 2024
8 of 12 checks passed
@Satrat Satrat deleted the logging-removal branch July 1, 2024 20:14
markmc pushed a commit to markmc/llm-compressor that referenced this pull request Nov 13, 2024
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.

4 participants