mirror of
https://github.com/milvus-io/milvus.git
synced 2026-10-12 02:19:10 +00:00
master
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chun HanandMrPresent-Han
|
33d4d826f3 |
enhance: expose compaction lifecycle observability (#53308)
issue: #53307 Protocol dependency: milvus-io/milvus-proto#668 (merged). ## What changed - Reuse the existing RPCs to expose retained segment lifecycle metadata and direct compaction sources; retain the existing sealed/loaded segment reporting contract without adding Growing segments to GetQuerySegmentInfo. - Expose collection-scoped retained compaction tasks, with complete task identity, public type/state enums, source/target sets, and failure reasons, while preserving manual job-ID lookup. - Preserve failure reasons across worker cleanup without changing task cleanup or metadata GC semantics. - Add seven **L3-only** data-integrity workloads: fixed V2/V3 Insert, fixed V2/V3 Import, V2 add/drop-field evolution through MixCompaction with schema bump disabled, V3 schema evolution, and V2-to-V3-to-V2 storage transitions. - Validate deterministic full-row/full-field data against an independent oracle, fenced persistence/serving checkpoints, and actual lineage transitions, allowing either automatic or manual compaction. - Provide `tests/scripts/ci_compaction_integrity.sh` as a dedicated, single-invocation, serial entry; leave the shared `tests/scripts/ci_e2e.sh` unchanged from the PR base. - Retain successful audit records only for the dedicated workload marker, preserving ordinary tests' JSON, HTML, and worker-report formats. ## Compatibility and execution boundary - Omitted segment states preserve the existing persistent-state behavior; task and dropped-segment history are subject to existing retention/GC, not a fixed response-size or record-count limit. - These workloads are not added to per-PR E2E or ordinary Nightly pipelines. - The dedicated job must own the deployment/config namespace exclusively, install a compatible PyMilvus build, and provide connection options plus a unique `CI_LOG_PATH`. - The entry enforces `-n 0`, the L3 workload marker, and explicit opt-in, while forwarding connection options, filters, and pytest's exit status. - Insert/Import workloads use three rounds by default; insert load units are batches, not a promise of a physical segment count, and dense vector dimensions are 16. ## Observability semantics and follow-ups - **Segment listing is not an atomic snapshot.** The Proxy first calls `GetSegmentsByStates`, then `GetSegmentInfo`; returned details are filtered again against the requested states (or the legacy default: Flushing, Flushed, Sealed). A segment whose fetched state no longer matches is omitted, and a segment becoming eligible after ID enumeration may be absent. Consumers must not treat a single response as a consistent lifecycle snapshot. - **Existing lookup errors are preserved.** `IncludeUnHealthy` is enabled only when Dropped is explicitly requested. In the default healthy-only path, a selected source becoming Dropped before detail lookup still returns `SegmentNotFound`, as before; that error is not converted into a successful partial list. A history segment being GC'd between the two RPCs can also fail the lookup. - **Aggregate `Completed` means no running tasks, not universal success.** This preserves the existing aggregation semantics: all-failed/all-timeout retained tasks, or an empty task set, can report Completed. Inspect each task's `state` and `failure_reason` for its outcome; proving an actual data transition additionally requires lineage, active-set adoption, and serving-set evidence. This PR does not redefine task retry or cleanup behavior. - **History capacity is a follow-up.** Dropped-segment queries enumerate the retained history and fetch its details, while collection-scoped compaction queries return retained tasks across triggers. Neither path currently provides pagination or a fixed count/size cap, so scan, copy, and response costs grow with the retained record count despite retention/GC. Pagination and capacity controls for both paths are deferred; they must preserve the ability to enumerate complete retained history rather than silently truncate lineage or task evidence. ## Previously recorded verification The results below are earlier recorded runs, including the original six-workload suite before the V2 DDL workload was added; the documentation/review-response update did not rerun tests. - 22 server-independent CI boundary tests passed, covering ordinary/special audit output, worker report merging, scoped execution guards, and the dedicated entry's argument/exit-code behavior. - 20 differential checks against the PR-base logging plugin produced byte-identical ordinary worker JSON, report JSON, and HTML across all five result outcomes. - 14 existing DataIntegrity and etcd-helper unit tests passed. - The three original L1 DataIntegrity parameter instances passed with `-n 2 --dist loadgroup` in 59.39 seconds; their merged successful JSON records retained the original metadata-only shape. - Ruff lint/format, shell syntax, and scoped PR-base whitespace checks passed; the shared runner is byte-identical to the PR base. - The new dedicated entry ran all six workloads serially against the existing local cluster: **6 passed in 3373.52 seconds**, with three rounds for each Insert/Import workload and the storage-transition workload, plus five DDL checkpoints over 25,000 rows. - All six successful reports retained their audit evidence (403 structured events), including every expected round/DDL checkpoint, full segment/task snapshots, serving frontiers, and matching expected/retrieved PK digests. ## Design and job entry - [Data-integrity design contract](https://github.com/MrPresent-Han/milvus/blob/codex/compaction-e2e-observability/docs/design-docs/design_docs/compaction_data_integrity_e2e_design.md) - [Dedicated serial entry](https://github.com/MrPresent-Han/milvus/blob/codex/compaction-e2e-observability/tests/scripts/ci_compaction_integrity.sh) Signed-off-by: MrPresent-Han <[email protected]> Co-authored-by: MrPresent-Han <[email protected]> |
||
|
|
f929d68ba6 |
test: clear the ruff baseline in tests/ so the lint gate reflects the PR under review (#52115)
issue: #52114 Makes `ruff check` and `ruff format --check` pass over all of `tests/`, so the `Ruff (changed files only)` job stops failing PRs for violations they did not introduce (most recently #51903, which got 69 errors from an 8-line change). Almost all of this is mechanical. The behavioural changes are called out individually below: one `NameError` fix, three shadowed duplicate definitions, 13 assertion gaps, and two undefined names that only became visible once star imports were expanded. ### Layout Seven reviewable commits: | commit | what | files | |---|---|---| | 1 | `ruff format` | 170 | | 2 | `ruff check --fix` (safe fixes only, no `--unsafe-fixes`) | 156 | | 3 | F821 fix + `tests/ruff.toml` | 4 | | 4 | three shadowed duplicate definitions | 4 | | 5 | close 13 assertion gaps behind F841 | 7 | | 6 | expand star imports, re-enable F403/F405/F821 | 33 | | 7 | enable E402, E712, E722 | 16 | 1745 violations → 0. Only `tests/` is touched. ### The undefined-name bug `chaos/test_chaos_memory_stress.py`: `TestMemoryStressReplica.prepare_collection` referenced bare `nb` and `dim`. Those are class attributes, never module-level names, so the fixture raised `NameError` the moment it ran — and the line directly above it already used `self.dim`. Changed to `self.nb` / `self.dim`. This was the only F821 in `tests/`; **the rule stays enabled**. ### Three shadowed duplicate definitions Three places define the same name twice in the same scope. Python keeps the later one, so the earlier definition is dead while still reading as live code — a test that looks like it exists and never runs. **`restful_client/testcases/test_vector_operations.py` — `TestDeleteVector`** ```python @pytest.mark.L0 class TestDeleteVector(TestBase): # line 778 — shadowed, never ran def test_delete_vector_default(...) # 2x2 parametrized @pytest.mark.L1 class TestDeleteVector(TestBase): # line 838 — wins def test_delete_vector_with_invalid_api_key(...) def test_delete_vector_with_invalid_collection_name(...) def test_delete_vector_with_non_primary_key(...) ``` The second holds only negative cases, and the file already uses `TestInsertVectorNegative` / `TestSearchVectorNegative`, so this is a copy whose class name was never updated. Renamed it to `TestDeleteVectorNegative`. **This is the one to look at:** it brings back an `L0` case, so 4 parametrizations of `test_delete_vector_default` start running in PR CI. They have never executed, so if the restful delete path has drifted since they were written, this is where it surfaces. **`python_client/testcases/test_compaction.py` — `test_compact_after_delete`** Defined twice in `TestCompactionParams`, covering different things: the shadowed one (L3, `@parametrize("delete_pos", [1, tmp_nb // 2])`) covers *partial* deletes plus a compaction-plan check; the surviving one (L2) deletes everything. There is already a separate `test_compact_after_delete_all`, so partial-delete coverage existed only in the shadowed case and was not running. Renamed it to `test_compact_after_delete_partial`; the surviving definition is untouched. **`python_client/utils/util_pymilvus.py` — `add_vector_field`** Defined twice with different signatures: ```python def add_vector_field(entities, is_normal=False): # line 455 — shadowed ... entities.append(field) return entities def add_vector_field(nb, dimension=default_dim): # line 527 — wins field_name = gen_unique_str() field = {...} # built, then discarded return field_name # returns only the name ``` Removed the second — both the broken one and the one that was winning. Grepped the whole repo: **neither definition has a caller**; every `add_vector_field` hit is something else (`test_milvus_client_add_vector_field` test methods, the `Op.add_vector_field` enum member in `chaos/checker.py`, a `Checker` method of the same name). Dead-code removal, not an API change. **Net effect on what executes:** `test_delete_vector_default` ×4 (**L0**) and `test_compact_after_delete_partial` ×2 (L3) start running; nothing stops running; the 3 negative cases keep running under the new class name. Computed by parsing each file before and after and resolving shadowing the way Python does, not by reading the diff. F811 is now clean across `tests/` with no per-file suppressions. ### Star imports expanded, F403/F405/F821 re-enabled 33 of 307 files used `from utils.util_pymilvus import *` and friends. Inside those, pyflakes cannot tell a typo from a star-imported name — it downgrades everything unresolved to F405, and **F821 never fires**. The `NameError` fixed in commit 3 was only catchable because that file happens not to use a star import. The 379 F405 hits were only **58 distinct names**. All are now explicit, resolved by walking the star modules in source order (later import wins, matching Python). Names that `util_pymilvus` merely re-exports come from their real source now — `DataType` from `pymilvus`, `np` from `numpy`, `log` from `utils.util_log` — rather than being laundered through it. Turning F821 on immediately caught **two real undefined names**, both in `rate_limit/test_rate_limit.py` (L3, so it has never run in CI): ```python for collection_num in range(collectionRateLimit): # no such name # ^ the parametrize arg is `collection_rate_limit`, and it is # a string ("10"/"500"), so range() needs int(). # The same test uses the correct name 30 lines below. mil.uninstall(release_name, namespace=NAMESPACE) # no such name # ^ MilvusOperator.uninstall has exactly this signature, and other suites do the # same under `mic`/`milvus_op` — the instantiation was just missing. ``` Both fixed. `test_milvus_client_data_integrity_new.py` needed 15 `noqa` markers instead: its `prepare_data` fixture injects those names via `globals()[field] = ...`, which no static analysis can follow. That block has other problems — a `float is None` check against the builtin, an always-true `if "is null" or "IS NULL" in expression`, leftover `binbin_debug1` logging — but rewriting fixture logic does not belong here; noted in #52116. ### 13 assertion gaps closed Of ~80 F841 hits, most are harmless scratch values. 13 were a call whose result was bound and then never checked. Highlights: - `test_milvus_client_database.py`: two tests apply a non-existent property key (`data.replica.number`, note `data.` not `database.`), describe the database, and drop it without checking anything. Their docstrings say `actual: run successfully` / `nothing changed` — the intent was to prove nothing changed. Now asserted. - `test_milvus_client_alias.py`: two assertions were commented out, **and they were commented out for a reason** — `list_aliases()[0]` returns a dict, so `assert alias in aliases` would have tested dict keys. Rewritten against `[0]["aliases"]` (how the rest of the file does it) and re-enabled. In the skipped `alias_default` test the calls were also missing the required `collection_name` argument. The remaining ~65 F841 stay, and the rule stays off. ### E402, E712, E722 also enabled **E722** (37 sites): mechanical `except:` → `except Exception:`. Checked the two ways this could change behaviour, both empty: no bare-except try-block can raise `SystemExit`/`KeyboardInterrupt`, and **none calls `pytest.skip`/`fail`/`xfail`**. That second one matters — pytest's `Skipped` derives from `BaseException`, so a bare except silently eats it while `except Exception` would not. 0 of 37, so behaviour is unchanged. **E402** (5 sites): imports that were just in the wrong place — `import re` sat 70 lines into `check/param_check.py`, and the two `resource_group` modules imported `rich` after their module-level constants. Nothing to do with `sys.path`, which is what an earlier version of this description wrongly claimed. **E712** (5 sites), gone through one at a time rather than bulk-rewritten: | site | value | change | |---|---|---| | `test_milvus_client_partition.py` | `has_partition()` | → `is False` — stricter than `not result`, which would pass on `None` | | `test_restart.py` | JSON status flag | → `is True` | | `test_restart.py` | local `break_flag` | → plain truth test, it is a real bool | | `test_query.py` | wrapper `check_result` | → `is True` | | `test_milvus_client_ttl.py` | `hit.get(bool_field)` | **kept as `== False` + noqa** | That last one is the reason this rule cannot be bulk-fixed: `.get()` returns `None` for a missing field, so `not ...` would pass exactly where the assertion should fail, and `is False` would break on a numpy bool. ### What ruff.toml still disables | rule | n | reason | |---|---|---| | F841 | ~65 | tests deliberately keep call results; the 13 that were genuine assertion gaps are fixed above | | UP031 | 95 | pure style rewrite that catches no bugs, and 27 sites use `% (...)` tuples where it is not equivalent | Everything else pyflakes/pycodestyle checks is on, including `F401`, `F811` and `F821`, so new code cannot introduce dead, duplicate or undefined names. Tracked in #52116. ### Verification Not just "ruff is green" — the mechanical passes were checked for semantic drift: - **Format pass:** parsed all 307 files before and after and compared ASTs with string constants normalized away → **zero structural differences**. The only AST-visible deltas are 146 string-constant changes, every one whitespace-only (docstring indentation / trailing whitespace); zero non-whitespace string changes. - **Import removals:** 142 imports were dropped. For each, checked whether the bound name still appears anywhere in its file. 115 have no remaining reference at all. The 27 that still matched textually were each read by hand and are all comments (`# pdb.set_trace()`, the commented-out checker dict in `test_load_with_checker.py`), docstring prose (`"input random collection name"`, `"Tuple[List[Dict]]"`), string literals (`"Collection ID"`, the `"config"` dict key), a different binding (`from time import sleep`), or attribute access through another module (`ct.CaseLabel`, `np.random`). **No real reference was removed.** - **Python version:** the pyupgrade rewrites emit `datetime.UTC` (3.11+) and `X | None` (3.10+). `tests/.python-version` is 3.12, `tests/scripts/python-env.sh` defaults to 3.12, and `tests/docker/Dockerfile` is `python:3.12-bullseye`. - **`compileall`** over all 307 files under Python 3.12 passes. - No external reference to `TestDeleteVector`, `test_compact_after_delete`, or `add_vector_field` exists anywhere in the repo (`*.py`, `*.sh`, `*.yaml`, `*.json`). ### Conflict surface Of the 100 currently-open PRs, 9 touch `tests/*.py` and only **2** overlap with this change: #51903 (3 of its 5 files) and #51856 (1 file). The other 7 touch files that were already format-clean. Worth merging promptly to keep that number small. `.pre-commit-config.yaml` needs no change — it already runs with `--config=tests/ruff.toml`, so local hooks and CI stay in sync. --------- Signed-off-by: xiaofanluan <[email protected]> Co-authored-by: xiaofanluan <[email protected]> Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
yanliang567andClaude Opus 4.5
|
7b78c09e98 |
enhance: distinguish xfail and xpass in test report plugin (#47588)
## Summary This PR enhances the Python test log filter plugin to properly distinguish between xfail and xpass test outcomes, which were previously not categorized separately. - **xfail**: Tests marked with `@pytest.mark.xfail` that failed as expected - **xpass**: Tests marked with `@pytest.mark.xfail` that passed unexpectedly (needs attention) Related to #47253 ## Changes - Detect xfail/xpass using pytest's `wasxfail` attribute - Add `xfail` and `xpass` categories to `test_stats` tracking - Update JSON report with xfail/xpass in summary and tests sections - Update HTML report with: - New summary cards for xfail (purple) and xpass (pink) - XPass section with full logs (since unexpected passes need investigation) - XFail section with reason only (expected behavior, just tracking) - Support parallel test runs with worker data merging for xfail/xpass ## Test plan - [x] Python syntax validation passes - [ ] Run tests with xfail markers to verify correct categorization - [ ] Verify JSON report contains xfail/xpass sections - [ ] Verify HTML report displays xfail/xpass with correct styling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: yanliang567 <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> |
||
|
yanliang567
|
bdb56f6053 |
enhance: Optimize Python test logging from 60MB to 3-5MB per run (#47253)
## Summary This PR optimizes Python test logging to reduce log file size from 60MB+ to 3-5MB per test run (90%+ reduction) while preserving full debugging information for failed tests. related issue: #47256 ### Key Improvements - **Conditional Logging**: PASSED tests save only metadata, FAILED tests preserve complete logs - **Memory Buffering**: Eliminates runtime disk I/O (no file writes during test execution) - **Unified Reports**: Generates 2 formats - `test_report.json`: AI-friendly structured data - `test_report.html`: Human-readable with color-coded logs - **Worker Parallelism**: Full support for `pytest -n` with data merging ### Performance Impact | Metric | Before | After | Improvement | |--------|--------|-------|-------------| | **Log Size** (15K tests) | 60 MB | 3-5 MB | **-90%+** | | **Memory Usage** (6 workers) | ~50 MB | ~450 MB | +400 MB | | **CPU Overhead** | N/A | 0.0004% | Negligible | | **Runtime I/O** | 75 seconds | 75 ms | **-99.9%** | ### Changes **New Files:** - `plugin/log_filter.py`: Conditional log handler with per-test memory buffers - `plugin/__init__.py`: Plugin package initialization **Modified Files:** - `config/log_config.py`: Simplified config (only JSON/HTML paths) - `conftest.py`: Register log filter plugin - `utils/util_log.py`: Removed redundant file handlers - `utils/api_request.py`: Optimized API request logging - `check/param_check.py`: Truncate long lists in error messages - `common/common_func.py`: Removed verbose logging - `pytest.ini`: Added log filter plugin **Cleanup:** - `milvus_client/test_add_field_feature.py`: Removed unused variable ### Test Plan - [x] Syntax validation: All Python files pass `py_compile` - [x] Module imports: All modules load successfully - [x] Data structures: Verified serialization without report/buffer objects - [x] Log configuration: Confirmed only necessary attributes present - [x] Small-scale test (17 tests): ✅ Generated correct JSON + HTML reports - [x] Performance analysis: Memory/CPU profiling for 15K test scenario ### Example Output **test_report.html** (human-readable): - Beautiful color-coded UI - Failed tests with full error traceback - Expandable log sections by level (debug/info/warning/error) **test_report.json** (AI-friendly): ```json { "metadata": { "total_tests": 15000, ... }, "summary": { "passed": 13500, "failed": 1500, ... }, "tests": { "failed": [ { "id": "test.py::test_func", "error": { "type": "AssertionError", ... }, "logs": { "debug": [...], "error": [...] } } ] } } ``` ### Memory Safety For the standard scenario (15K tests, 10% failure, 6 workers): - Worker memory: ~53 MB each (318 MB total) - Main process peak: ~112 MB - Total system: **< 450 MB** ✅ Safe High-risk scenarios (>30% failure or >50K tests) may require optional streaming optimizations (documented in performance analysis). ### Backward Compatibility - ✅ No breaking changes to external APIs - ✅ Existing test commands work unchanged - ✅ Compatible with all pytest plugins 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Claude Opus 4.5 <[email protected]> Signed-off-by: yanliang567 <[email protected]> |