[Disco] Propagate structlog configuration to disco workers #16618
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hzfengsy
reviewed
Feb 23, 2024
| ) | ||
|
|
||
| from . import executor | ||
| from . import disco |
Member
There was a problem hiding this comment.
Not sure if it's safe when runtime is not compiled with disco.
Contributor
Author
There was a problem hiding this comment.
Hmm, good point. The tvm._ffi.register_object("runtime.disco.*") calls would raise an error if disco isn't available.
This was primarily to ensure that all packaged funcs generated by tvm.runtime.disco were available within the disco sub-process. I'll try a build without disco enabled, and see what is required to make sure that the module import works either way.
Contributor
Author
There was a problem hiding this comment.
Looks like disco is enabled by default, and is always included in libtvm_runtime.so (CMakeLists.txt link. As a result, it should always be safe to import tvm.runtime.disco.
If we later add a cmake flag to disable disco, we should be able to gate this import behind a check on tvm.support.libinfo()[cmake_flag].
Prior to this commit, while `structlog.configure(...)` would only impact log statements generated in the main process. Any workers started with `disco.session.ProcessSession` do not inherit the `structlog` configuration. While `disco.session.ThreadedSession` would inherit the `structlog` configuration, it would also inherit process-specific CUDA variables. This commit updates `disco.session.ProcessSession` to explicitly propagate any `structlog` configuration to child processes. This implementation intentionally avoids introducing a new dependency for TVM. If the `structlog` package is not available, the config propagation is skipped.
Lunderberg
force-pushed
the
disco_propagate_structlog_configuration_to_workers
branch
from
fc2f93a to
208d444
Compare
Lunderberg
added a commit
to octoml/mlc-llm
that referenced
this pull request
Mar 11, 2024
Required for compatibility with TVM PR apache/tvm#16618. Can be removed after upstream `structlog` hynek/structlog#603 lands. This is a backport of the OLLM PR https://github.com/octoml/ollm/pull/409, and is not needed after the MLC-serve migration.
masahi
approved these changes
Mar 11, 2024
masahi
pushed a commit
to octoml/mlc-llm
that referenced
this pull request
Mar 11, 2024
Required for compatibility with TVM PR apache/tvm#16618. Can be removed after upstream `structlog` hynek/structlog#603 lands. This is a backport of the OLLM PR https://github.com/octoml/ollm/pull/409, and is not needed after the MLC-serve migration.
Lunderberg
deleted the
disco_propagate_structlog_configuration_to_workers
branch
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Mar 13, 2024
This is a follow-up to apache#16618, which propagates the `structlog` configuration to disco worker processes. For configurations that use `structlog.stdlib` to integrate `structlog` with the stdlib `logging` module, this integration must also be forwarded.
Lunderberg
added a commit
that referenced
this pull request
Mar 27, 2024
This is a follow-up to #16618, which propagates the `structlog` configuration to disco worker processes. For configurations that use `structlog.stdlib` to integrate `structlog` with the stdlib `logging` module, this integration must also be forwarded.
thaisacs
pushed a commit
to thaisacs/tvm
that referenced
this pull request
Apr 3, 2024
) Prior to this commit, while `structlog.configure(...)` would only impact log statements generated in the main process. Any workers started with `disco.session.ProcessSession` do not inherit the `structlog` configuration. While `disco.session.ThreadedSession` would inherit the `structlog` configuration, it would also inherit process-specific CUDA variables. This commit updates `disco.session.ProcessSession` to explicitly propagate any `structlog` configuration to child processes. This implementation intentionally avoids introducing a new dependency for TVM. If the `structlog` package is not available, the config propagation is skipped.
thaisacs
pushed a commit
to thaisacs/tvm
that referenced
this pull request
Apr 3, 2024
This is a follow-up to apache#16618, which propagates the `structlog` configuration to disco worker processes. For configurations that use `structlog.stdlib` to integrate `structlog` with the stdlib `logging` module, this integration must also be forwarded.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit,
structlog.configure(...)would only impact log statements generated in the main process, while workers started withdisco.session.ProcessSessionwould not inherit thestructlogconfiguration. Whiledisco.session.ThreadedSessionwould inherit thestructlogconfiguration, it would also inherit process-specific CUDA variables.This commit updates
disco.session.ProcessSessionto explicitly propagate anystructlogconfiguration to child processes. This implementation intentionally avoids introducing a new dependency for TVM. If thestructlogpackage is not available, the config propagation is skipped.