refactor: remove hardcoded /workspace paths across tests #3888
+86
−24
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
…code Replace hardcoded /workspace paths with dynamic workspace directory detection using the centralized get_workspace_dir() utility from dynamo.common.utils.paths. Changes: - benchmarks/profiler/utils/config.py: Use get_workspace_dir() for planner workingDir - tests/serve/common.py: Remove duplicate workspace detection, import from paths module - tests/serve/launch/template_verifier.py: Replace hardcoded path with WORKSPACE_DIR - tests/serve/test_sglang.py: Use WORKSPACE_DIR for sglang_dir fallback - tests/serve/test_trtllm.py: Improve path formatting consistency - tests/serve/test_vllm.py: Replace hardcoded paths with WORKSPACE_DIR (including commented code) - tests/utils/managed_deployment.py: Use get_workspace_dir() in main() - components/src/dynamo/common/utils/__init__.py: Export paths module This enables tests to work correctly both inside and outside Docker containers. Signed-off-by: Keiven Chang <[email protected]>
tedzhouhk
approved these changes
Oct 25, 2025
| mainContainer=Container( | ||
| image="my-registry/dynamo-runtime:my-tag", # placeholder | ||
| workingDir="/workspace/components/src/dynamo/planner", | ||
| workingDir=f"{get_workspace_dir()}/components/src/dynamo/planner", |
There was a problem hiding this comment.
There might be a small issue here if the profiler's image (where this python code is running in) has a different workspace dir than workspace dir of the image where this generated DGD is using.
With that said, the current approach is no better (and it fails for sgl-wideep image 100% sure).
Suggested change
| workingDir=f"{get_workspace_dir()}/components/src/dynamo/planner", | |
| # todo: get the accurate workspace dir in the docker image instead of the workspace dir of the profiler image | |
| workingDir=f"{get_workspace_dir()}/components/src/dynamo/planner", |
There was a problem hiding this comment.
Do you know how the profiler image is generated, vs. the normal Dockerfile.{vllm,sglang,trtllm} image?
krishung5
approved these changes
Oct 25, 2025
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.
Overview:
This PR removes all hardcoded
/workspacepaths from test and profiler code. This enables tests and profiler utilities to work correctly both inside various Docker containers (dev vs. local-dev) and in native development environments.Details:
dynamo.common.utils.paths.get_workspace_dir()WORKSPACE_DIRconstant instead of hardcoded pathstests/serve/common.pyWhere should the reviewer start?
tests/serve/common.py- Simplified workspace detection by importing from paths modulebenchmarks/profiler/utils/config.py- Dynamic path usage in DgdPlannerServiceConfigtests/serve/test_vllm.py,tests/serve/test_sglang.py,tests/serve/test_trtllm.py/coderabbit profile chill