Make sure that dataset_transformation tests actually run in CI #1116
Conversation
Added detailed logging throughout the vLLM generation pipeline to diagnose why the benchmark script hangs: - Log prompt submission and queue sizes in benchmark_generators.py - Log actor_manager should_stop status and engine ready status - Log _prefetch_worker iterations and request processing in vllm_utils.py - Log async task creation, completion accumulation, and result queue operations This will help identify where in the pipeline the hang occurs (prompt queue, async tasks, completion queue, or results queue). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added detailed logging at critical points in the vLLM engine initialization pipeline to diagnose where the script hangs: - benchmark_generators.py: Log ActorManager creation and vLLM engine setup - vllm_utils.py create_vllm_engines: Log engine creation loop progress - LLMRayActor.__init__: Log each initialization step - _setup_and_start_async_engine: Log thread creation and startup sequence This will help identify whether the hang occurs during Ray actor creation, actor initialization, or vLLM AsyncLLMEngine startup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The script was hanging because two placement groups were being created: 1. One in setup_vllm_engines() that allocated the GPU 2. Another in create_vllm_engines() that waited forever for the same GPU The placement group created in setup_vllm_engines was only passed to create_vllm_engines when single_gpu_mode=True, causing create_vllm_engines to create a second placement group when single_gpu_mode=False. Fix: Always pass the placement group to create_vllm_engines, preventing the duplicate allocation attempt. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the testing for dataset_transformation.py by moving ad-hoc tests into a proper unittest suite, which is a great improvement for CI and maintainability. The changes also include significant enhancements to the benchmarking scripts, making them more configurable and robust. The updates to benchmark_generators.py and the new shell scripts for launching benchmarks are well-structured. I've found one minor semantic issue in how epoch_number is being set, which I've commented on. Overall, these are solid improvements to the project's testing and benchmarking infrastructure.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Hamish Ivison <hamishivi@gmail.com>
Co-authored-by: Hamish Ivison <hamishivi@gmail.com>
Previously, they only ran when someone manually ran them.
Note
Runs dataset_transformation unit tests in CI with a new test suite and updated workflow, and factors dataset config construction into a reusable helper.
8-Core-XL-Runner-Ubuntu-Latestand passHF_TOKENwhen runningpytestin.github/workflows/tests.yml.open_instruct/test_dataset_transformation.pywith unit tests for tokenizer equality, config-hash differences, and cached dataset parity (using small splits and temp caches).load_dataset_configs(...)and reuse it inget_cached_dataset_tulu_with_statistics(...).__main__block fromopen_instruct/dataset_transformation.py.Written by Cursor Bugbot for commit 4c7ef47. This will update automatically on new commits. Configure here.