Fix missing logs when calling oneshot #1446
Conversation
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
There was a problem hiding this comment.
Thanks @kelkelcheng for looking into this! One comment regarding your TODO, otherwise I think this looks great!
|
Looks like tests are failing for an unrelated reason, will look into it and then we can merge this in |
Got it, thanks for taking a look! |
|
Hi @brian-dellabetta, just wanted to check in and see if there’s any update on the test issue you mentioned earlier. Let me know if there's anything I can do to help move things along, thanks |
Hey @kelkelcheng , thanks for the ping. This is happening because forked repos don't have access to the HF_TOKEN we embed in the github workflow, and some of our PR checks currently use it. I need to move those to nightly tests. Now that the tracing PRs have landed, we can do that, just gotta figure out how in our CI/CD. Hoping to wrap that up today. Happy Friday! |
|
Once #1493 lands, we can rebase this and it should pass the checks |
SUMMARY: some tests in `tests/llmcompressor/transformers/tracing/test_models.py` requiring access to gated HF hub models will fail for community user PRs, because `HF_TOKEN` is not injected. This just skips the tests in question. This update allows #1446 and #1445 to pass GHA checks so that they can be merged in TEST PLAN: n/a Signed-off-by: Brian Dellabetta <[email protected]>
…e.py Signed-off-by: Kelvin Cheng <[email protected]>
Signed-off-by: Kelvin Cheng <[email protected]>
brian-dellabetta
force-pushed
the
kc/oneshot-log-issue
branch
from
8fbbb3c to
24b988c
Compare
This reverts commit a0adf83.
Fix missing logs when calling oneshot #1441
Summary:
When calling oneshot (e.g., in gemma2_example.py), a sparse_logs folder is created, but the log file inside is nearly empty. This is because the logging setup in lifecycle.py and state.py uses loguru, but oneshot itself didn’t configure loguru to write logs to a file. As a result, logs were not being saved properly.
This PR fixes the issue by adding a log file configuration directly in the oneshot init. This ensures loguru writes to the appropriate file by default.
To clean up the nearly empty log file, I removed the two lines from stage.py that were generating it. These messages appear to be redundant, as similar info is already captured by loguru.
Currently, I set the log path defaults to sparse_logs. Maybe we can consider making the log file path an optional input, allowing users to disable file logging by passing None. Let me know what you guys think.
Test Plan:
Ran gemma2_example.py after the changes:
A proper log file was generated in sparse_logs
The original nearly empty log file was no longer created
Also ran make test and got the same results as the current main.