Skip to content

Fix logging.info not appearing #8384

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

Merged
merged 4 commits into from
Feb 12, 2025
Merged

Fix logging.info not appearing #8384

merged 4 commits into from
Feb 12, 2025

Conversation

jackzhxng
Copy link
Contributor

Summary

Logs at the info level were not appearing for export_llama, omitting useful logs for intermediate export states with --verbose. This also fixes for when a debugger is being used.

Test plan

Ran export_llama instructions from README

@jackzhxng jackzhxng requested a review from tarun292 February 11, 2025 19:12
Copy link

pytorch-bot bot commented Feb 11, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8384

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures

As of commit a9652e8 with merge base 1308d4d (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 11, 2025
@jackzhxng jackzhxng requested a review from lucylq February 11, 2025 21:16
Copy link
Contributor

@lucylq lucylq left a comment

Choose a reason for hiding this comment

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

lgtm

as a note, seems like this is overriding the warning log in xnnpack partitioner:
https://github.com/pytorch/executorch/blob/main/backends/xnnpack/partition/xnnpack_partitioner.py#L26

could potentially move that to info?

@jackzhxng
Copy link
Contributor Author

jackzhxng commented Feb 11, 2025

Yeah it's overriding the earliest logging levels which is set somewhere in the export_llama dependencies. Maybe one of the recursive module imports recently had a logging level initialized to WARNING or lower.

Since global logging level can only be configured once, should we remove the logging.basicConfig calls in modules such as https://github.com/pytorch/executorch/blob/main/extension/llm/export/builder.py#L46? If they want to specify a level they should probably be using named loggers cc @dbort

@jackzhxng jackzhxng merged commit 1e03da2 into main Feb 12, 2025
43 of 46 checks passed
@jackzhxng jackzhxng deleted the jz/export-llama-logging branch February 12, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants