Repository navigation
Conversation
ds_nvme_tune prints its recommendation as a ds_config "aio" section, but it spelled the parallelism key num_threads and wrote single_submit and overlap_events as the strings "true"/"false". get_aio_config reads intra_op_parallelism (num_threads was never a key it accepted), so the tuned value was dropped for the default of 1, and the flags reached the aio handle constructors as strings, which declare both parameters as const bool. Build the section from the aio config constants with real booleans, and replace the stale thread_count key in the config reference. Add a test that feeds the printed section back through get_aio_config. Signed-off-by: Sonica-B <shreyaa.boyane@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ds_nvme_tuneends by printing its recommendation as a ds_config"aio"section. That section does not surviveget_aio_config, the parser DeepSpeed uses for theaioblock:num_threads, butget_aio_configreadsintra_op_parallelism, so the tuned value is dropped without a warning and the default1is used;single_submitandoverlap_eventsare printed as the JSON strings"true"/"false", andget_aio_confighands them unchanged to theaio_handle/gds_handleconstructors, which declare both parametersconst bool.This PR makes the tuner print the keys and types that the parser reads, replaces the stale
thread_countkey in the config reference, and adds a CPU-only test that feeds the printed section back throughget_aio_config.How the printed section reaches the runtime
flowchart TD sweep["`**ds_nvme_tune** sweep_main one log per sweep point, e.g. read_single_overlap_ftd1_d32_bs1M_p8.txt`"] gen["`generate_main picks the fastest point convert_to_param builds the printed aio section`"] subgraph master_box ["master"] m_out["`**printed** #quot;num_threads#quot;: 8 #quot;single_submit#quot;: #quot;true#quot; #quot;overlap_events#quot;: #quot;true#quot;`"] m_parse["`get_aio_config reads intra_op_parallelism, never num_threads`"] m_cfg["`**result** intra_op_parallelism = 1 (default) single_submit = 'true' (str) overlap_events = 'true' (str)`"] m_out -->|pasted into ds_config| m_parse --> m_cfg end subgraph pr_box ["this PR"] b_out["`**printed** #quot;intra_op_parallelism#quot;: 8 #quot;single_submit#quot;: true #quot;overlap_events#quot;: true`"] b_parse["`get_aio_config reads intra_op_parallelism`"] b_cfg["`**result** intra_op_parallelism = 8 single_submit = True overlap_events = True`"] b_out -->|pasted into ds_config| b_parse --> b_cfg end handles["`aio_handle / gds_handle constructors (NVMe param and optimizer swappers, FastFileWriter) single_submit, overlap_events declared const bool`"] sweep --> gen gen --> m_out gen --> b_out m_cfg --> handles b_cfg --> handles classDef bad fill:#fdecea,stroke:#c62828,color:#000 classDef good fill:#e8f5e9,stroke:#2e7d32,color:#000 class m_out,m_cfg bad class b_out,b_cfg goodBefore and after (captured output)
Both runs use the same input: two synthetic sweep logs in the read and write log folders,
single_overlap_ftd1_d32_bs1M_p8(read 5.0 GB/s, write 4.0 GB/s) andblock_sequential_ftd1_d128_bs8M_p1(1.0 GB/s each). A standalone script (not part of this PR) callsgenerate_mainon them, which is the stepds_nvme_tuneruns after its sweep. The script prints the output, passes the printed section toget_aio_config, and compares the result with the winning sweep point. It does the same steps as the first case of the new test.On master (
fc7bc30), the script exits with code 1:With this PR, the script exits with code 0:
get_aio_configon masterget_aio_configwith this PRintra_op_parallelismp8, so 81(the default;num_threadsis never read)8single_submitsingle, so true'true'(a str)Trueoverlap_eventsoverlap, so true'true'(a str)Truequeue_depthd32, so 323232block_sizebs1M, so 104857610485761048576Root cause
convert_to_paramindeepspeed/nvme/perf_generate_param.pyhard-codes its own key names. Since #6560 added it, it has printednum_threadsand string flags. At that time the runtime key wasthread_count, and #7056 renamed it tointra_op_parallelismindeepspeed/runtime/swap_tensor/only, soget_aio_confighas never read the tuned value.get_aio_configreads a fixed set of keys withget_scalar_param: an unknown key is ignored and a value of the wrong type is returned as is.The string flags then reach the
aio_handle/gds_handleconstructors, whose pybind11 bindings (csrc/aio/py_lib/py_ds_aio.cpp,csrc/gds/py_lib/py_ds_gds.cpp) declaresingle_submitandoverlap_eventsasconst bool. pybind11'sboolcaster (in the pybind11 3.1.0 headers bundled with torch 2.14.1) acceptsTrue,False,Noneand objects whose type defines__bool__;strdoes not define__bool__. From that source, aTypeErroris expected when the swapper builds its handle. This was not observed, because the async_io op does not build on the Windows machine used here.The docs drifted the same way. On master, the sample output in
docs/_tutorials/deepnvme.mdalready shows"intra_op_parallelism": 8, which is not what the tool prints, together with the string flags. The config referencedocs/_pages/config-json.mdstill documents the pre-#7056 namethread_count, which is also ignored:get_aio_config({'aio': {'thread_count': 4}})returns'intra_op_parallelism': 1.Change
convert_to_parambuilds the section from theAIO_*constants indeepspeed/runtime/swap_tensor/constants.py, the same namesget_aio_configreads. It returns Python booleans, whichjson.dumpsprints astrue/false.generate_aio_paramusesAIOfor the section name. The new import is a module with no imports of its own.docs/_pages/config-json.md:thread_countbecomesintra_op_parallelismin the example and in the field heading.docs/_tutorials/deepnvme.md: the sampleds_nvme_tuneoutput showsfalse/trueinstead of"false"/"true".tests/unit/v1/nvme/test_ds_nvme_tune.py: it writes synthetic read and write sweep logs, runsgenerate_main, and passes the printed section throughget_aio_config. Two cases cover both flag values (single_overlap_ftd1_d32_bs1M_p8andblock_sequential_ftd1_d128_bs8M_p2). It fails on master and passes with this change. It needs no op. It is a new file becausetest_aio.pyskips at module level when the async_io op is unavailable.Testing
Hardware and software: one Intel Core Ultra 9 285H CPU, Windows 11 Home (10.0.26200), Python 3.11.9, torch 2.14.1+cpu (
torch.cuda.is_available()isFalse). Settings:DS_ACCELERATOR=cpu,DS_BUILD_OPS=0, DeepSpeed imported from the source tree. No accelerator or accelerator driver was used, and there was no NVMe device or libaio, so the async_io and GDS ops were not built.For the "before" rows, only
deepspeed/nvme/perf_generate_param.pywas reverted to master (git checkout master -- deepspeed/nvme/perf_generate_param.py); the new test file stayed in place.pytest -q tests/unit/v1/nvme/test_ds_nvme_tune.pypytest -q tests/unit/v1/nvmeyapf --diff(0.43.0) andflake8(5.0.4) on the two changed.pyfilescodespell(2.1.0) on the changed files and both doc pagesscripts/check-torchdist.pyandscripts/check-torchcuda.pyon the two changed.pyfilespython ci/tests_fetcher.py --base mastertests/unit/v1/nvme/test_ds_nvme_tune.pyThe 11 skips are the existing op tests:
test_aio.py,test_gds.py,test_pinned_manager.pyandtest_fast_file_writer_fd_close.pyskip at module level, and the 7 cases oftest_aio_gil.pyskip individually. All report that async_io or GDS is not compatible on this system. On Windows,scripts/check-license.pyreports a missing header for any file with CRLF line endings in the working tree, including untouched master files such asdeepspeed/nvme/perf_run_sweep.py. The committed blobs have LF endings and the standard header, so theformattingcheck in CI is the authoritative run. The full CPU unit suite was not run.Compatibility matrix
No GPU, NVMe or ZeRO-offload run was done, so no cell of the matrix (ZeRO stage, offload, precision, parallelism) was validated. The change is limited to the JSON that
ds_nvme_tuneprints, two doc pages and a new unit test.get_aio_configand every runtime consumer of theaiosection are unchanged, so a config that already spellsintra_op_parallelismwith boolean flags parses exactly as before.Not validated:
offload_param.device: nvme,partitioned_param_swapper.py), in fp32, fp16 or bf16.offload_optimizer.device: nvme,partitioned_optimizer_swapper.pyandpipelined_optimizer_swapper.py). The config reference documents NVMe offload as stage 3 only, so stages 0 to 2 have no NVMe cell.use_gds), andFastFileWritercheckpoint writing, which builds handles from the same section.ds_nvme_tunesweep, which needs Linux, libaio and an NVMe device.TypeErrorexpectation for string flags described in Root cause.Sibling paths checked and left unchanged
csrc/aio/py_test/aio_bench_generate_param.py, the legacy benchmark, has the sameconvert_to_parampattern: it printsthread_countand string flags. It is left out to keep this PR to the shippedds_nvme_tunepath and can be folded in if preferred.--gds, the sweep runs the GDS operator, but the printed section has no"use_gds": true, becausegenerate_mainreceives only the log directory.--flush_page_cachehelp text inperf_run_sweep.pydescribes the behavior without the flag ("Page cache will not be flushed ...").Duplicates and related work
Checked on 2026-10-25:
ds_nvme_tune,intra_op_parallelism,perf_generate_param,convert_to_paramordeepnvme tuning: only fix(nvme): run main() from the perf script __main__ blocks #8344.__main__block:generate_main()becomesmain()inperf_generate_param.py, andsweep_main()becomesmain()inperf_run_sweep.py. It does not touchconvert_to_param, the printed keys, the docs or the tests. Agit merge-treeof this branch and fix(nvme): run main() from the perf script __main__ blocks #8344's head6d4e2a9, with its parent92843adas the merge base, completes with no conflicts.ds_nvme_tune OR perf_generate_param OR convert_to_param: DeepNVMe perf tuning #6560 (merged, added the tuner) and fix(nvme): run main() from the perf script __main__ blocks #8344.aioparam section #998, a 2021 how-to thread on profiling NVMe for theaiosection with the legacycsrc/aio/py_testscripts. None of its comments mentionnum_threadsorintra_op_parallelism. No issue reports the dropped key or the string flags.