perf(imports): stop loading optional integrations to import grpo - #3552
perf(imports): stop loading optional integrations to import grpo#3552tianyi-zhang-02 wants to merge 2 commits into
Conversation
`import nemo_rl.algorithms.grpo` pulled in wandb, mlflow, swanlab, matplotlib,
fastapi and uvicorn. Every CLI entrypoint, every Ray actor and every unit-test
collection paid for all of them.
- utils/logger.py: mlflow, swanlab, wandb and matplotlib.pyplot become lazy
module proxies. They stay module-level names, so neither the call sites nor
the patch targets in tests/unit/utils/test_logger.py change.
- experience/{metric_utils,rollout_manager,rollouts}.py: `from wandb import
Table/Histogram`, one call site each.
- utils/memory_tracker.py: `from ray.scripts.scripts import memory_summary`.
This is Ray's CLI entrypoint and it is what actually pulls in fastapi and
uvicorn -- not the vLLM workers, which is why deferring those alone changes
nothing measurable.
- generation/vllm/{vllm_worker_async,vllm_sparse_refit}.py: fastapi and uvicorn
behind TYPE_CHECKING, with the affected annotations quoted so they are not
evaluated at def time.
Warm cache, median of 7 runs:
import nemo_rl.algorithms.grpo 7.71s -> 3.42s (-56%)
modules loaded 7098 -> 5203 (-27%)
and wandb, mlflow, fastapi and uvicorn are absent from sys.modules afterwards.
196 unit tests across tests/unit/utils/test_logger.py and tests/unit/experience/
pass before and after, with no test modified.
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
5b3c345 to
44e9b89
Compare
|
Force-pushed after rebuilding this on current main. Two things were wrong with the previous version, both worth recording: It silently reverted ~115 lines of the vLLM 0.25 migration. I had taken
The branch now differs from main by 11 lines in Re-measured on the rebuilt branch, warm cache, median of 7:
|
Quoting the fastapi annotations was wrong. FastAPI resolves handler annotations against endpoint.__globals__; with Request only under TYPE_CHECKING the ForwardRef never resolves, and instead of raising FastAPI demotes raw_request to a required query parameter. Every sparse-refit endpoint then answers 422 without any error in the logs. Reproduced with fastapi's TestClient: TYPE_CHECKING plus a quoted annotation gives 422 'query.raw_request Field required'; a runtime import inside setup_api_server with an unquoted annotation gives 200. Import Request next to the existing local JSONResponse import and unquote both handlers. Nested defs resolve annotations from the enclosing scope at def time, so this keeps the module-level import deferred, which was the point of the PR. Also fixes the two lint gates this PR was failing: ruff 0.9.9 format on vllm_worker_async.py and the isort rule on vllm_sparse_refit.py. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
What does this PR do ?
Stops
import nemo_rl.algorithms.grpofrom loading wandb, mlflow, swanlab, matplotlib, fastapi and uvicorn. None are needed unless the corresponding feature is used, but every CLI entrypoint, Ray actor and unit-test collection paid for all of them.Warm cache, median of 9 runs:
import nemo_rl.algorithms.grpoAfterwards
wandb,mlflow,fastapianduvicornare absent fromsys.modules. The seconds are from one laptop; the module count is deterministic and the number I would trust.What moved, in descending order of cost:
utils/logger.py—mlflow,swanlab,wandb,matplotlib.pyplotbecome lazy module proxies. This is most of the win (nemo_rl.utils.loggeralone: 5597 → 3409 modules).experience/{metric_utils,rollout_manager,rollouts}.py—from wandb import Table/Histogram, one call site each.utils/memory_tracker.py—from ray.scripts.scripts import memory_summary, used only inget_snapshot_str().generation/vllm/{vllm_worker_async,vllm_sparse_refit}.py—fastapi/uvicornbehindTYPE_CHECKING.Issues
Supersedes and closes #3437, which tried a piece of this and does not actually work — applying it alone leaves the module count at exactly 7150.
vllm_sparse_refit.py:29-31importsfastapiat module scope as well, and the vLLM workers are not the real source anyway:Usage
No config or API change.
nemo_rl.utils.logger.wandband friends resolve exactly as before.Before your PR is "Ready for review"
Additional Information
Why a proxy in
logger.pyrather than function-scope imports.tests/unit/utils/test_logger.pyhas ~100patch("nemo_rl.utils.logger.<name>")targets. Moving those imports into methods removes the module attribute and breaks 28 tests — I tried that first. Lazy module-level names mean neither the 15 call sites nor a single test changed.plt.Figureannotations are quoted so importing the module does not touch the proxy.No new tests. Behaviour is unchanged, and asserting "module X is not in
sys.modules" is easy to write but easy to make flaky. Happy to add a subprocess test pinning whichever of the four you want guarded.Verification. 195 unit tests across
tests/unit/utils/test_logger.py,tests/unit/experience/andtests/unit/algorithms/test_utils.pypass before and after, no test modified.ruff checkandruff format --checkclean; none of the seven files are inpyrefly.toml. I have not run the GPU or functional suites.cc. @RayenTian @yuki-97