Skip to content

Commit 7c633ca

Browse files
committed
chore: address PR review nits
- ci-checks.yml: fix echo alignment in status check job - TESTING.md: use `uv run --frozen pytest` instead of bare `pytest` - test_training_cpu.py: remove unused `fixture_stub_tokenizer_path` param - Makefile: use PYTEST_NO_XDIST_CMD instead of appending -n 0 to PYTEST_CMD; fix test-gpu-integration help text to match its actual recipe (e2e only) Made-with: Cursor
1 parent 6fd711e commit 7c633ca

4 files changed

Lines changed: 15 additions & 16 deletions

File tree

.github/workflows/ci-checks.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,9 @@ jobs:
178178
steps:
179179
- name: Check job results
180180
run: |
181-
echo "changes: ${{ needs.changes.result }}"
182-
echo "format: ${{ needs.format.result }}"
183-
echo "typecheck: ${{ needs.typecheck.result }}"
181+
echo "changes: ${{ needs.changes.result }}"
182+
echo "format: ${{ needs.format.result }}"
183+
echo "typecheck: ${{ needs.typecheck.result }}"
184184
echo "unit-test: ${{ needs.unit-test.result }}"
185185
echo "smoke-test: ${{ needs.smoke-test.result }}"
186186

Makefile

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,8 @@ test-smoke: ## Run CPU smoke tests (~few min, no GPU required)
185185
SMOKE_DIR := tests/smoke
186186
.PHONY: test-smoke-gpu
187187
test-smoke-gpu: ## Run GPU smoke tests (requires CUDA)
188-
# -n 0 disables xdist. Groups are split for GPU memory isolation.
188+
# Uses PYTEST_NO_XDIST_CMD (-n 0) because CUDA device-side asserts poison
189+
# xdist workers. Groups are split for GPU memory isolation.
189190
#
190191
# When adding a new GPU smoke test file:
191192
# - Train-only (no vLLM): add pytest.mark.requires_gpu -> auto-discovered below
@@ -194,23 +195,21 @@ test-smoke-gpu: ## Run GPU smoke tests (requires CUDA)
194195
# - Downloads from Hub: also add pytest.mark.smollm2 (or similar) -> auto-discovered below
195196
#
196197
# 1) Train-only tests share a process (no vLLM, safe to batch).
197-
$(PYTEST_CMD) $(SMOKE_DIR)/ -n 0 -m "requires_gpu and not vllm and not smollm2 and not unsloth"
198+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/ -m "requires_gpu and not vllm and not smollm2 and not unsloth"
198199
# 2) Each vLLM test file gets its own process -- vLLM pre-allocates all GPU
199200
# memory and never releases it within a process.
200-
$(PYTEST_CMD) $(SMOKE_DIR)/test_nss_generation_gpu.py -n 0
201-
$(PYTEST_CMD) $(SMOKE_DIR)/test_nss_resume_gpu.py -n 0
202-
$(PYTEST_CMD) $(SMOKE_DIR)/test_nss_structured_gen_gpu.py -n 0
203-
$(PYTEST_CMD) $(SMOKE_DIR)/test_nss_timeseries_gpu.py -n 0
201+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/test_nss_generation_gpu.py
202+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/test_nss_resume_gpu.py
203+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/test_nss_structured_gen_gpu.py
204+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/test_nss_timeseries_gpu.py
204205
# 3) SmolLM2 (Hub download + vLLM) and Unsloth (patches transformers) are marker-isolated.
205-
$(PYTEST_CMD) $(SMOKE_DIR)/ -n 0 -m "requires_gpu and smollm2"
206-
$(PYTEST_CMD) $(SMOKE_DIR)/ -n 0 -m "requires_gpu and unsloth"
206+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/ -m "requires_gpu and smollm2"
207+
$(PYTEST_NO_XDIST_CMD) $(SMOKE_DIR)/ -m "requires_gpu and unsloth"
207208

208209

209210
E2E_TEST_FILE := $(NSS_ROOT_PATH)/tests/e2e/test_safe_synthesizer.py
210211
.PHONY: test-gpu-integration
211-
test-gpu-integration: ## Run GPU integration tests (smoke GPU + e2e)
212-
# -n 0 disables xdist: CUDA device-side asserts poison the worker, cascading to all subsequent tests.
213-
# Separate invocations: (1) local tiny-model tests, (2) SmolLM2 Hub test, (3) Unsloth (process-isolated from DP).
212+
test-gpu-integration: ## Run GPU e2e tests (default + DP configs)
214213
pushd $(NSS_ROOT_PATH) && \
215214
$(PYTEST_CMD) $(E2E_TEST_FILE) -k default && \
216215
$(PYTEST_CMD) $(E2E_TEST_FILE) -k dp

tests/TESTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ make test-ci-container # CI tests in a Linux container (Docker/Podman)
3232
Run a single test:
3333

3434
```bash
35-
pytest tests/path/test_file.py::test_name -vvs -n0
35+
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0
3636
```
3737

3838
Test runner: `uv run --frozen pytest -n auto --dist loadscope -vv`

tests/smoke/test_training_cpu.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ def test_dp_training_one_step(tiny_model, stub_tokenizer, tiny_training_dataset_
122122
assert len(trainer.state.log_history) > 0
123123

124124

125-
def test_training_example_assembler(iris_df, stub_tokenizer, fixture_stub_tokenizer_path, tmp_path):
125+
def test_training_example_assembler(iris_df, stub_tokenizer, tmp_path):
126126
"""Exercises: NSS data preparation pipeline (TrainingExampleAssembler)."""
127127
from nemo_safe_synthesizer.config import SafeSynthesizerParameters
128128

0 commit comments

Comments
 (0)