Skip to content

fix: Heavy logging by TensorRT-LLM monolith worker - #347

Merged
kthui merged 3 commits into
dynamo/trtllm_llmapi_v1from
jacky-trtllm-monolith-logging
Mar 25, 2025
Merged

kthui merged 3 commits into
dynamo/trtllm_llmapi_v1from
jacky-trtllm-monolith-logging

Conversation

@kthui

@kthui kthui commented Mar 22, 2025 •

Copy link
Copy Markdown
Contributor

Overview:

Stop heavy logging by TensorRT-LLM monolith worker.

Details:

N/A

Where should the reviewer start?

Start with MR !51 for adding CI.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: N/A

@copy-pr-bot

copy-pr-bot Bot commented Mar 22, 2025

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@grahamking grahamking closed this Mar 24, 2025
@kthui

kthui commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

@grahamking this fix is required for testing this example on the CI, otherwise each run from this example will accumulate > 10 GB log file.

@kthui kthui reopened this Mar 24, 2025
@kthui kthui changed the title fix: Stop heavy logging by TensorRT-LLM monolith worker fix: Heavy logging by TensorRT-LLM monolith worker Mar 24, 2025
@kthui
kthui requested a review from nnshah1 March 24, 2025 15:31
@grahamking

Copy link
Copy Markdown
Contributor

@grahamking this fix is required for testing this example on the CI, otherwise each run from this example will accumulate > 10 GB log file.

Would redirecting the output to /dev/null work? python <the_example> 2>&1 > /dev/null

@kthui

kthui commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Would redirecting the output to /dev/null work? python <the_example> 2>&1 > /dev/null

This will fix the logging issue, but if there is any failure by the worker, then we won't get any log for debug?

Comment thread examples/trtllm/monolith/worker.py Outdated
from dynamo.runtime import DistributedRuntime, dynamo_endpoint, dynamo_worker

logger.set_level("debug")
logger.set_level("info")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't there any more places which are setting debug logging in the examples? For example, router and disagg worker pieces?
I would run a find and replace for the logger.set_level.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Everything are at "info" level, except this occurrence, i.e.
debug_level

@tanmayv25
tanmayv25 self-requested a review March 25, 2025 03:25

@NVShreyas NVShreyas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you change it such that we can use an env variable to control log level for all TRTLLM component files? Its pretty tedious to keep changing it when developing.

@kthui

kthui commented Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

Could you change it such that we can use an env variable to control log level for all TRTLLM component files?

Sure, it will read from DYNAMO_TRTLLM_LOG_LEVEL if set. If not set, it will default to info.

@grahamking

Copy link
Copy Markdown
Contributor

Could you change it such that we can use an env variable to control log level for all TRTLLM component files?

Sure, it will read from DYNAMO_TRTLLM_LOG_LEVEL if set. If not set, it will default to info.

Our other environment variables all start DYN_. Could you use that?

@NVShreyas NVShreyas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks

@kthui

kthui commented Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

Our other environment variables all start DYN_. Could you use that?

Yes, the prefix is updated to DYN_.

@kthui

kthui commented Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

Merging this PR, as the TensorRT-LLM tests on pipeline #25899729 are passing.

@kthui
kthui merged commit db35630 into dynamo/trtllm_llmapi_v1 Mar 25, 2025
@kthui
kthui deleted the jacky-trtllm-monolith-logging branch March 25, 2025 18:29

This branch was previously deployed

1 inactive deployment
GITLAB — ea74f83b Deployed Mar 25, 2025 by kthui
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