From 5131e52984ed244bc4b8ef808843396f40549fd0 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Tue, 29 Sep 2026 11:15:00 -0700 Subject: [PATCH 1/9] feat(admin): MPM presence in growth-report via --mpm (OPIK-8411) Chargeback carries no MPM data, so `--mpm` collects it client-side from existing endpoints and merges it into each chargeback workspace: - /api/mpm/v3/workspaces answers, in one call, every workspace the API key's user belongs to. - The REST v2 model registry covers the rest: list each workspace's models, then read each model's monitored flag (parallel, 8 workers). Both apply the same is_monitored predicate server-side. A workspace whose lookup fails is reported as unknown, never as zero. Adds MPM workspaces / Monitored models KPIs, an MPM models column in the by-workspace table, an MPM leaderboard, mpm_enabled / num_monitored_models columns in growth_workspaces.csv, and mpm_workspaces / total_monitored_models / mpm_workspaces_unchecked org KPIs. Off by default because of the per-model requests. Co-Authored-By: Claude Opus 5.5 (1M context) --- README-ADMIN.md | 10 +- README.md | 2 + cometx/cli/admin.py | 13 ++ cometx/cli/admin_growth_csv.py | 29 +++- cometx/cli/admin_growth_mpm.py | 182 ++++++++++++++++++++++ cometx/cli/admin_growth_report.py | 137 ++++++++++++++--- cometx/cli/admin_growth_users.py | 70 ++++++++- tests/unit/test_admin_growth_csv.py | 54 ++++++- tests/unit/test_admin_growth_mpm.py | 202 +++++++++++++++++++++++++ tests/unit/test_admin_growth_report.py | 85 +++++++++++ tests/unit/test_admin_growth_users.py | 63 ++++++++ 11 files changed, 817 insertions(+), 30 deletions(-) create mode 100644 cometx/cli/admin_growth_mpm.py create mode 100644 tests/unit/test_admin_growth_mpm.py diff --git a/README-ADMIN.md b/README-ADMIN.md index d4d31da..fc367ee 100644 --- a/README-ADMIN.md +++ b/README-ADMIN.md @@ -172,6 +172,7 @@ report is scoped to just those workspaces. - **`--no-open`**: Don't automatically open the generated HTML file after generation. - **`--csv-dir DIR`**: Also write Glue-ready CSV fact tables (`growth_users.csv`, `growth_workspaces.csv`, `growth_org_kpis.csv`) into `DIR`. `DIR` is created if it doesn't exist. - **`--no-html`**: Skip the HTML report. Requires `--csv-dir` (otherwise there is nothing to write, and the command errors out). +- **`--mpm`**: Include MPM presence — whether each workspace uses MPM and how many models it monitors (registry models flagged `is_monitored`). Chargeback has no MPM data, so this is collected from `/api/mpm/v3/workspaces` (one call, covering the workspaces the API key's user belongs to) and, for the rest, the REST v2 model registry (one request per registry model). Use an org-admin key so every workspace is visible. Off by default because of the extra requests. - **`--chargeback-report FILE`**: Read the chargeback report from a local JSON file (as saved by `cometx admin chargeback-report`) instead of calling the chargeback endpoint. Useful for CSV-only pipelines that already have a saved snapshot, or for re-running without re-fetching it. Note this skips the *chargeback* request only — the report still queries `/api/admin/service-accounts` to classify accounts, and falls back to a name-pattern heuristic if that request fails (`service_account_source` records which was used). ### The two time concepts @@ -209,9 +210,9 @@ cometx admin growth-report --active-window 30d --leaderboard-top-n 10 \ The report generates a single self-contained HTML file containing: -- An **Organization overview (chargeback)** section with org-wide KPIs (Total workspaces, Total EM projects, New in {window} (% of base), Active workspaces %), a workspace platform-mix chart (EM / Opik / both / neither), workspace total-vs-active and added-vs-deleted charts, and a by-workspace table. +- An **Organization overview (chargeback)** section with org-wide KPIs (Total workspaces, Total EM projects, New in {window} (% of base), Active workspaces %, plus MPM workspaces and Monitored models with `--mpm`), a workspace platform-mix chart (EM / Opik / both / neither), workspace total-vs-active and added-vs-deleted charts, and a by-workspace table (with an MPM models column under `--mpm`). - A **Users** section with Total / Active (`--active-window`) / Active % / New in {window} KPIs, plus active-vs-total, adoption-rate, per-capability, and user-churn charts. -- A **Leaderboards** section ranking workspaces (by experiments and EM projects, exact from chargeback) and users (by Opik spans and EM activity), as top-N and active-aware bottom-N. Metrics with no data are omitted. +- A **Leaderboards** section ranking workspaces (by experiments and EM projects, exact from chargeback; by MPM monitored models under `--mpm`) and users (by Opik spans and EM activity), as top-N and active-aware bottom-N. Metrics with no data are omitted. - A **Personal vs Service accounts** section splitting experiments / data / spans between personal and service accounts. Service accounts are identified from the admin service-accounts API when available, falling back to a labeled regex heuristic; the source is shown in the panel hint. ### Caveats @@ -219,6 +220,7 @@ The report generates a single self-contained HTML file containing: - **Chargeback is required.** The whole report is derived from the admin chargeback report; without admin access the command errors out (non-zero exit). - **Workspace "created" is a proxy** — the earliest member `createdAt` in that workspace, since chargeback has no workspace-creation timestamp. The added-vs-deleted "deleted" series is also a best-effort proxy (all members removed) and typically reads ~0. - **"Total projects" counts EM projects only** — chargeback's per-workspace `projects[]` covers Experiment Management. Opik projects and MPM aren't represented there (Opik appears only as a per-user span count; MPM is absent), so the platform mix uses an Opik per-user proxy and excludes MPM. +- **MPM comes from `--mpm`, not chargeback.** It reflects which models are monitored *now*, so past months can't be reconstructed; a trend builds up from monthly `--csv-dir` exports. A workspace whose MPM lookup fails is reported as unknown (empty in the CSV), never as zero, and the report says how many workspaces were checked. `nb_models_registered` from the monthly usage report is not used: it counts every registry model and overstates MPM adoption. - **The people layer degrades independently.** If the chargeback payload parses but a section's inputs are missing, a warning is printed and only that section is dropped — the rest of the report still generates. ### CSV export @@ -249,7 +251,9 @@ numeric/date types instead of typing everything as `string`. this table. The column exists so it is present and typed for a Glue crawler. Use the `deleted_users` KPI to see how many were excluded. -**`growth_workspaces.csv`**: `report_date, workspace, member_count, num_projects, num_experiments, data_mb` +**`growth_workspaces.csv`**: `report_date, workspace, member_count, num_projects, num_experiments, data_mb, mpm_enabled, num_monitored_models` + +`mpm_enabled` is `1`/`0` (so `SUM(mpm_enabled)` counts MPM workspaces) and `num_monitored_models` a count; both are empty unless `--mpm` was given and that workspace could be checked. With `--mpm`, `growth_org_kpis.csv` also gets `mpm_workspaces`, `total_monitored_models`, and `mpm_workspaces_unchecked`. **`growth_org_kpis.csv`**: `report_date, metric_name, metric_value, metric_unit, metric_text` — long format so new metrics arrive as new rows without ever changing the Glue schema. `metric_unit` is one of `count`, `percent`, `megabytes`, `label`. diff --git a/README.md b/README.md index 5c2b761..5c075a1 100644 --- a/README.md +++ b/README.md @@ -605,6 +605,7 @@ cometx admin growth-report [WORKSPACE ...] * `--exclude-personal` - Drop workspaces whose name matches `--personal-pattern` from the chargeback data (default: off; no effect without `--personal-pattern`). * `--personal-pattern REGEX` - Regex used with `--exclude-personal` to identify personal-workspace names to drop, e.g. `'^user-'` (default: none). * `--no-open` - Don't automatically open the generated HTML file after generation. +* `--mpm` - Include MPM presence (monitored models per workspace), collected from the MPM and model-registry APIs. Use an org-admin API key; off by default because it makes one request per registry model in workspaces the key's user isn't a member of. **Two time concepts:** * `--units` controls chart *granularity* — every chart shows the full all-time history bucketed at this resolution. @@ -628,6 +629,7 @@ that section is dropped. * **Chargeback is required** — the whole report is derived from the admin chargeback report; without admin access the command errors out. * **Workspace "created" is a proxy** — the earliest member `createdAt`, since chargeback has no workspace-creation timestamp. The added-vs-deleted "deleted" series is also a best-effort proxy (all members removed) and typically reads ~0. * **"Total projects" counts EM projects only** — chargeback's per-workspace `projects[]` covers Experiment Management; Opik projects and MPM aren't represented (Opik appears only as a per-user span count; MPM is absent). +* **MPM needs `--mpm`.** Chargeback has no MPM data; `--mpm` collects monitored models per workspace from the MPM and model-registry APIs (see README-ADMIN.md). **Examples:** ``` diff --git a/cometx/cli/admin.py b/cometx/cli/admin.py index 582f637..178a79f 100644 --- a/cometx/cli/admin.py +++ b/cometx/cli/admin.py @@ -496,6 +496,7 @@ def get_parser_arguments(parser): cometx admin growth-report --csv-dir ./out cometx admin growth-report --csv-dir ./out --no-html cometx admin growth-report --chargeback-report report.json --csv-dir ./out + cometx admin growth-report --mpm --csv-dir ./out """ growth_parser = subparsers.add_parser( "growth-report", @@ -583,6 +584,17 @@ def get_parser_arguments(parser): default=False, action="store_true", ) + growth_parser.add_argument( + "--mpm", + default=False, + action="store_true", + help=( + "Include MPM presence (monitored models per workspace), collected " + "from the MPM and model-registry APIs. Needs an org-admin API key " + "to see every workspace; makes one request per registry model in " + "workspaces the key's user is not a member of" + ), + ) growth_parser.add_argument( "--chargeback-report", default=None, @@ -916,6 +928,7 @@ def admin(parsed_args, remaining=None): csv_dir=parsed_args.csv_dir, no_html=parsed_args.no_html, chargeback=preloaded, + mpm=parsed_args.mpm, ) except GrowthReportError as e: # `_exception_text`, not `str(e)`: a broken `__str__` would diff --git a/cometx/cli/admin_growth_csv.py b/cometx/cli/admin_growth_csv.py index 9831491..a06ca20 100644 --- a/cometx/cli/admin_growth_csv.py +++ b/cometx/cli/admin_growth_csv.py @@ -19,7 +19,10 @@ import os import tempfile -from cometx.cli.admin_growth_users import _looks_like_service_account +from cometx.cli.admin_growth_users import ( + _looks_like_service_account, + workspace_org_totals, +) USERS_HEADER = [ "report_date", @@ -48,6 +51,12 @@ "num_projects", "num_experiments", "data_mb", + # MPM presence (OPIK-8411), appended per the stable-column-order rule. + # `mpm_enabled` is 1/0 (not true/false) so SUM(mpm_enabled) counts MPM + # workspaces directly; both are empty when MPM was not collected + # (no --mpm) or could not be determined for that workspace. + "mpm_enabled", + "num_monitored_models", ] ORG_KPIS_HEADER = [ @@ -191,6 +200,8 @@ def build_workspaces_rows(ws_records, report_date): _num_or_empty(w.num_projects), _num_or_empty(w.num_experiments), _num_or_empty(w.data_mb), + "" if w.mpm_enabled is None else int(w.mpm_enabled), + _num_or_empty(w.num_monitored_models), ] for w in ws_records ] @@ -236,6 +247,7 @@ def collect_org_kpis( active_window_days, scope=None, excluded_personal_count=0, + mpm_status=None, ): """Flatten the report's org-level numbers into (name, value, unit) triples. @@ -338,6 +350,21 @@ def collect_org_kpis( ("total_experiments", sum(w.num_experiments for w in ws_records), "count") ) kpis.append(("total_data_mb", sum(w.data_mb for w in ws_records), "megabytes")) + # MPM presence (--mpm). Omitted (not zero) when not collected. + # `mpm_workspaces_unchecked` says how many workspaces the totals could not + # cover, so a partial run is never mistaken for a complete one. + org = workspace_org_totals(ws_records) + if org["mpm_workspaces"] is not None: + kpis.append(("mpm_workspaces", org["mpm_workspaces"], "count")) + kpis.append(("total_monitored_models", org["monitored_models"], "count")) + if mpm_status is not None: + kpis.append( + ( + "mpm_workspaces_unchecked", + mpm_status["total"] - mpm_status["checked"], + "count", + ) + ) if split is not None: for bucket in ("personal", "service"): diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py new file mode 100644 index 0000000..307a4e4 --- /dev/null +++ b/cometx/cli/admin_growth_mpm.py @@ -0,0 +1,182 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- +# **************************************** +# __ +# _________ ____ ___ ___ / /__ __ +# / ___/ __ \/ __ `__ \/ _ \/ __/ |/_/ +# / /__/ /_/ / / / / / / __/ /__> < +# \___/\____/_/ /_/ /_/\___/\__/_/|_| +# +# +# Copyright (c) 2024 Cometx Development +# Team. All rights reserved. +# **************************************** +"""MPM presence for `cometx admin growth-report --mpm` (OPIK-8411). + +The chargeback report carries no MPM data, so this collects it client-side +from existing endpoints and merges it into the chargeback payload's +`workspaces[]` as `mpmEnabled` / `monitoredModels` -- the shape +`admin_growth_users.parse_workspaces` reads. + +A model counts as MPM-monitored when the registry flags it `is_monitored` +(and it is not pipeline-generated, a production-model reference, or deleted). +Both sources below apply exactly that predicate server-side: + +1. `GET /api/mpm/v3/workspaces` -- one call returning every workspace the + API key's user is a MEMBER of, with its monitored models. +2. For the remaining workspaces (an org admin need not be a member), the REST + v2 registry: list the workspace's models, then read each model's details + for its monitored flag. Org admins may read any workspace this way, + private models included. One request per model, so it runs in a pool. + +A workspace whose lookup fails is reported as unknown (`None`), never as +"no MPM": a false zero would read as a workspace that stopped using MPM. +`nb_models_registered` (the monthly usage report) is deliberately NOT used -- +it counts every registry model and overstates MPM adoption. +""" + +from __future__ import annotations + +import concurrent.futures + +from cometx.utils import admin_api_url + +MPM_WORKSPACES_PATH = "/api/mpm/v3/workspaces" +DEFAULT_MAX_WORKERS = 8 + + +def _fetch_member_workspaces(api) -> "dict[str, list[dict]] | None": + """`mpm/v3/workspaces` -> {workspace_name: [{"id", "name"}, ...]} for the + workspaces the caller belongs to. `None` on any failure (MPM disabled on + the deployment, network error, unexpected shape): the registry path then + covers every workspace instead.""" + try: + url = admin_api_url(api.config["comet.url_override"], MPM_WORKSPACES_PATH) + response = api._client.get( + url, headers={"Authorization": api.api_key}, params={} + ) + payload = response.json() + out = {} + for ws in payload["workspaces"]: + name = ws.get("workspaceName") + if not name: + continue + out[name] = [ + {"id": m.get("modelId"), "name": m.get("modelName")} + for m in (ws.get("models") or []) + if isinstance(m, dict) + ] + return out + except Exception: + return None + + +def _is_monitored(details) -> "bool | None": + """The monitored flag from a registry-model details response. The Java + field is `boolean isMonitored` without an explicit `@JsonProperty`, so + Jackson may serialize it as `monitored`; accept either. `None` when + neither is a real bool (the answer is unknown, not False).""" + if not isinstance(details, dict): + return None + for key in ("isMonitored", "monitored"): + value = details.get(key) + if isinstance(value, bool): + return value + return None + + +def _registry_monitored_models( + api, workspaces, max_workers=DEFAULT_MAX_WORKERS +) -> "dict[str, list[dict] | None]": + """Monitored models per workspace via the REST v2 registry. Two parallel + phases -- list every workspace's models, then read every model's details + -- so neither many small workspaces nor one large one runs serially. + + A workspace is `None` when its listing fails, any of its detail lookups + fails, or a model's flag cannot be read: a partial list would silently + undercount.""" + + def list_models(ws): + return ws, api._client.get_registry_models(ws) or [] + + def check(pair): + ws, name = pair + return ws, name, _is_monitored(api._client.get_registry_model_details(ws, name)) + + out: "dict[str, list[dict] | None]" = {} + ids: "dict[tuple, str]" = {} + pairs = [] + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as pool: + listed = {pool.submit(list_models, ws): ws for ws in workspaces} + for future in concurrent.futures.as_completed(listed): + ws = listed[future] + try: + _ws, models = future.result() + except Exception: + out[ws] = None + continue + out[ws] = [] + for m in models: + name = m.get("modelName") if isinstance(m, dict) else None + if name: + ids[(ws, name)] = m.get("registryModelId") + pairs.append((ws, name)) + + checked = {pool.submit(check, pair): pair for pair in pairs} + for future in concurrent.futures.as_completed(checked): + ws, name = checked[future] + if out.get(ws) is None: + continue # already failed + try: + _ws, _name, flag = future.result() + except Exception: + flag = None + if flag is None: + out[ws] = None + elif flag: + out[ws].append({"id": ids.get((ws, name)), "name": name}) + + # Stable order regardless of completion order. + for ws, models in out.items(): + if models: + models.sort(key=lambda m: m["name"]) + return out + + +def fetch_mpm_presence( + api, workspace_names, max_workers=DEFAULT_MAX_WORKERS +) -> "dict[str, list[dict] | None]": + """Monitored models per workspace: {name: [{"id", "name"}, ...]}, or + {name: None} where it could not be determined. Workspaces in + `mpm/v3/workspaces` are answered from that single call; the rest fall + back to the registry.""" + names = list(dict.fromkeys(n for n in workspace_names if n)) + member = _fetch_member_workspaces(api) or {} + presence = {n: member[n] for n in names if n in member} + remaining = [n for n in names if n not in presence] + if remaining: + print( + "Checking MPM models in %d workspace(s) via the model registry..." + % len(remaining) + ) + presence.update( + _registry_monitored_models(api, remaining, max_workers=max_workers) + ) + return presence + + +def apply_mpm_presence(chargeback, presence) -> dict: + """Return a copy of `chargeback` whose workspaces carry `mpmEnabled` / + `monitoredModels` from `presence`. Workspaces that are unknown (`None`) + or absent from `presence` are left without the fields, which + `parse_workspaces` reads as "not reported".""" + workspaces = [] + for w in chargeback.get("workspaces") or []: + models = presence.get(w.get("name")) + if models is None: + workspaces.append(w) + else: + workspaces.append( + {**w, "mpmEnabled": bool(models), "monitoredModels": list(models)} + ) + return {**chargeback, "workspaces": workspaces} diff --git a/cometx/cli/admin_growth_report.py b/cometx/cli/admin_growth_report.py index 76c9dbc..7a1fb31 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -28,6 +28,7 @@ import re from cometx.cli.admin_growth_csv import collect_org_kpis, write_growth_csvs +from cometx.cli.admin_growth_mpm import apply_mpm_presence, fetch_mpm_presence from cometx.cli.admin_growth_render import build_html, write_html from cometx.cli.admin_growth_users import ( _extract_licensed_users, @@ -39,6 +40,7 @@ churn_series, classify_accounts, em_user_breakdown_series, + mpm_reported, opik_user_breakdown_series, parse_users, parse_workspaces, @@ -334,6 +336,7 @@ def generate_growth_report( no_html=False, chargeback=None, report_date=None, + mpm=False, ): reporter = GrowthReporter( api, @@ -343,6 +346,7 @@ def generate_growth_report( leaderboard_top_n=leaderboard_top_n, exclude_personal=exclude_personal, personal_pattern=personal_pattern, + mpm=mpm, ) report_data = reporter.build(workspaces, chargeback=chargeback) @@ -398,8 +402,13 @@ def __init__( leaderboard_top_n=5, exclude_personal=False, personal_pattern=None, + mpm=False, ): self.api = api + # --mpm: collect MPM presence client-side (see admin_growth_mpm). + self.mpm = mpm + # {"checked": n, "total": m} after an --mpm build, else None. + self._mpm_status = None self.window = window self.units = units self.active_window = active_window @@ -478,9 +487,12 @@ def build(self, workspaces, chargeback=None): before = len((chargeback.get("workspaces") or [])) chargeback = self._filter_personal_chargeback(chargeback) excluded_personal_count = before - len((chargeback.get("workspaces") or [])) + scope = set(workspaces) if workspaces else None + self._mpm_status = None + if self.mpm: + chargeback = self._add_mpm_presence(chargeback, scope) print("Building report...") now_ms = int(now.timestamp() * 1000) - scope = set(workspaces) if workspaces else None report_data = self._assemble_report_data( chargeback, window, @@ -500,6 +512,43 @@ def build(self, workspaces, chargeback=None): ) return report_data + def _add_mpm_presence(self, chargeback, scope): + """Merge client-side MPM presence into the chargeback workspaces (only + those in `scope`, when given, to avoid needless per-model calls), and + record how many could actually be checked.""" + names = [ + w.get("name") + for w in (chargeback.get("workspaces") or []) + if w.get("name") and (scope is None or w.get("name") in scope) + ] + print("Collecting MPM presence for %d workspace(s)..." % len(names)) + presence = fetch_mpm_presence(self.api, names) + checked = sum(1 for n in names if presence.get(n) is not None) + self._mpm_status = {"checked": checked, "total": len(names)} + if checked < len(names): + print( + "Warning: could not determine MPM presence for %d of %d " + "workspace(s); they are reported as unknown, not as no MPM." + % (len(names) - checked, len(names)) + ) + return apply_mpm_presence(chargeback, presence) + + def _mpm_note(self, ws_records): + """Short provenance text for MPM figures in hints/KPI subs.""" + if not mpm_reported(ws_records): + return ( + "MPM not collected (run with --mpm)" + if not self.mpm + else "MPM could not be determined" + ) + status = self._mpm_status + if status and status["checked"] < status["total"]: + return "MPM checked in %d/%d workspaces" % ( + status["checked"], + status["total"], + ) + return "MPM shown separately" + def _empty_export_reason(self, scope, excluded_personal_count): """A block reason when the filters left nothing to export, else None. @@ -640,21 +689,31 @@ def _build_window_block(self, window, count_before): def _workspace_chargeback_table(self, ws_records): """Org-wide by-workspace table from chargeback (top 20 by experiments): - projects, experiments and data logged per workspace.""" + projects, experiments and data logged per workspace, plus monitored + MPM models when MPM presence was collected (--mpm).""" rows = sorted(ws_records, key=lambda w: -w.num_experiments)[:20] + with_mpm = mpm_reported(ws_records) + headers = ["Workspace", "Members", "Projects", "Experiments", "Data (MB)"] + if with_mpm: + headers.append("MPM models") + out_rows = [] + for w in rows: + row = [ + w.name, + len(w.members), + w.num_projects, + _num(w.num_experiments), + _num(round(w.data_mb)), + ] + if with_mpm: + row.append( + "" if w.num_monitored_models is None else w.num_monitored_models + ) + out_rows.append(row) return { "title": "By workspace (org-wide, chargeback)", - "headers": ["Workspace", "Members", "Projects", "Experiments", "Data (MB)"], - "rows": [ - [ - w.name, - len(w.members), - w.num_projects, - _num(w.num_experiments), - _num(round(w.data_mb)), - ] - for w in rows - ], + "headers": headers, + "rows": out_rows, } @staticmethod @@ -777,9 +836,9 @@ def _build_unified_section( ) # Org-wide platform mix (chargeback): EM-only / Opik-only / both / - # neither. MPM is not in chargeback, so it's excluded; Opik is a - # per-user proxy (a member's Opik usage is attributed to all their - # workspaces). + # neither. MPM has its own KPIs rather than being folded in here; Opik + # is a per-user proxy (a member's Opik usage is attributed to all + # their workspaces). if ws_records: mix = platform_mix(ws_records, people_users or []) mix_rows = [ @@ -795,7 +854,7 @@ def _build_unified_section( "title": "Workspace platform mix", "hint": ( "org-wide (chargeback); Opik is a per-user proxy; " - "MPM not represented in chargeback" + + self._mpm_note(ws_records) ), "data": {"rows": mix_rows}, } @@ -831,6 +890,31 @@ def _build_unified_section( "sub": f"{wa['active']}/{wa['total']} active", }, ] + # MPM presence (OPIK-8411, --mpm): registry models flagged + # `is_monitored`. Only shown when collected -- rendering 0 otherwise + # would read as "no MPM adoption". + if org["mpm_workspaces"] is not None: + total_ws = org["workspaces"] + pct = round(100 * org["mpm_workspaces"] / total_ws) if total_ws else 0 + kpis.append( + { + "label": "MPM workspaces", + "value": org["mpm_workspaces"], + "sub": f"{pct}% of workspaces", + } + ) + kpis.append( + { + "label": "Monitored models", + "value": org["monitored_models"], + "sub": ( + self._mpm_note(ws_records) + if self._mpm_status + and self._mpm_status["checked"] < self._mpm_status["total"] + else "MPM (is_monitored)" + ), + } + ) return { "title": "Organization overview (chargeback)", "window_chip": self._window_label(window), @@ -1114,8 +1198,8 @@ def _workspace_rollup(users, value_fn): def _build_leaderboards_section(self, users, ws_records): """Top-N / bottom-N workspace and user leaderboards, all org-wide from - chargeback. Experiments and projects are ranked EXACTLY from the - chargeback per-workspace numbers; Opik spans from the chargeback + chargeback. Experiments, projects and MPM models are ranked EXACTLY + from the chargeback per-workspace numbers; Opik spans from the chargeback per-user rollup (a proxy: a member's spans are attributed to each of their workspaces). Bottom-N is active-aware (strictly-positive, ascending). Empty metrics are omitted; returns `None` when there is @@ -1172,6 +1256,20 @@ def emit_ws(slug, label, totals, hint): org_exact_hint, ) + # MPM monitored models: exact, per-workspace from chargeback (only when + # MPM presence was collected; emit_ws skips an empty dict). + if ws_records and mpm_reported(ws_records): + emit_ws( + "ws-mpm-models", + "MPM monitored models", + { + w.name: w.num_monitored_models + for w in ws_records + if w.num_monitored_models + }, + org_exact_hint, + ) + # Opik spans: org-wide per-user rollup proxy. if users: emit_ws( @@ -1447,6 +1545,7 @@ def _assemble_report_data( active_window_days, scope=scope, excluded_personal_count=excluded_personal_count, + mpm_status=self._mpm_status, ), ) except Exception as exc: diff --git a/cometx/cli/admin_growth_users.py b/cometx/cli/admin_growth_users.py index 7542a3b..c5766a2 100644 --- a/cometx/cli/admin_growth_users.py +++ b/cometx/cli/admin_growth_users.py @@ -719,13 +719,60 @@ class WorkspaceRecord: data_mb: float num_projects: int members: "tuple" # member usernames (immutable, matching frozen=True) + # MPM presence (OPIK-8411, merged in by admin_growth_mpm under --mpm). + # `None` means not collected or not determinable -- distinct from + # "checked, and zero". Defaults keep + # the record constructible without MPM data. + mpm_enabled: "bool | None" = None + num_monitored_models: "int | None" = None + monitored_models: "tuple" = () # monitored model names, when listed + + +def _parse_mpm(w: dict) -> "tuple[bool | None, int | None, tuple]": + """Read a chargeback workspace's MPM presence fields. + + `monitoredModels` may be a list (of model dicts or names; its length is + the count) or already a number. `mpmEnabled` is taken as given when it is + a real bool, otherwise inferred from the count. Returns + (mpm_enabled, num_monitored_models, monitored_model_names); all-`None` + when the server reports neither field.""" + raw = w.get("monitoredModels") + names: tuple = () + count: "int | None" = None + if isinstance(raw, list): + count = len(raw) + names = tuple( + str(n) + for n in ( + ( + (m.get("name") or m.get("modelName") or m.get("id")) + if isinstance(m, dict) + else m + ) + for m in raw + ) + if n + ) + elif isinstance(raw, (int, float)) and not isinstance(raw, bool): + count = int(raw) + enabled = w.get("mpmEnabled") + if not isinstance(enabled, bool): + enabled = None if count is None else count > 0 + return enabled, count, names + + +def mpm_reported(workspaces: "list[WorkspaceRecord]") -> bool: + """True when MPM presence is known for any workspace. False means it was + not collected: callers should say so rather than show zero.""" + return any(w.mpm_enabled is not None for w in workspaces) def parse_workspaces(chargeback: dict) -> "list[WorkspaceRecord]": """Parse `chargeback["workspaces"]` into `WorkspaceRecord`s. `projects` may be a list (its length is the project count) or already a number; both are tolerated. These are EM/experiment projects -- chargeback does not carry - Opik projects or MPM models.""" + Opik projects. MPM presence (`mpmEnabled` / `monitoredModels`) is parsed + when present (merged in by `admin_growth_mpm`); see `_parse_mpm`.""" out = [] for w in (chargeback or {}).get("workspaces") or []: projects = w.get("projects") @@ -738,6 +785,7 @@ def parse_workspaces(chargeback: dict) -> "list[WorkspaceRecord]": members = tuple( m.get("userName") for m in (w.get("members") or []) if m.get("userName") ) + mpm_enabled, num_monitored, monitored = _parse_mpm(w) out.append( WorkspaceRecord( name=w.get("name"), @@ -745,18 +793,33 @@ def parse_workspaces(chargeback: dict) -> "list[WorkspaceRecord]": data_mb=w.get("totalSizeInMb") or 0.0, num_projects=num_projects, members=members, + mpm_enabled=mpm_enabled, + num_monitored_models=num_monitored, + monitored_models=monitored, ) ) return out def workspace_org_totals(workspaces: "list[WorkspaceRecord]") -> dict: - """Org-wide totals across all workspaces (from chargeback).""" + """Org-wide totals across all workspaces (from chargeback). + + `mpm_workspaces` / `monitored_models` are `None` when MPM presence was not + collected, so a run without --mpm never reads as zero adoption. + Monitored models come from the registry `is_monitored` flag, NOT + `nb_models_registered` (which counts every registry model).""" + reported = mpm_reported(workspaces) return { "workspaces": len(workspaces), "projects": sum(w.num_projects for w in workspaces), "experiments": sum(w.num_experiments for w in workspaces), "data_mb": sum(w.data_mb for w in workspaces), + "mpm_workspaces": ( + sum(1 for w in workspaces if w.mpm_enabled) if reported else None + ), + "monitored_models": ( + sum(w.num_monitored_models or 0 for w in workspaces) if reported else None + ), } @@ -770,7 +833,8 @@ def platform_mix( Opik = a member has `opik_span_count > 0`. This is a PROXY: chargeback carries Opik usage only per-user, so a user active in several workspaces has their Opik usage attributed to all of them (possible false positives). - MPM is absent from chargeback and is NOT classified here.""" + MPM is deliberately NOT folded in (it would make 8 buckets and break + comparability with earlier reports); it has its own KPIs instead.""" opik_ws = set() for u in users: if (u.opik_span_count or 0) > 0: diff --git a/tests/unit/test_admin_growth_csv.py b/tests/unit/test_admin_growth_csv.py index 01bfde3..bc9b3ed 100644 --- a/tests/unit/test_admin_growth_csv.py +++ b/tests/unit/test_admin_growth_csv.py @@ -171,10 +171,7 @@ def test_service_accounts_fall_back_to_heuristic(): def test_workspaces_header_and_rows(): - from cometx.cli.admin_growth_csv import ( - WORKSPACES_HEADER, - build_workspaces_rows, - ) + from cometx.cli.admin_growth_csv import WORKSPACES_HEADER, build_workspaces_rows from cometx.cli.admin_growth_users import WorkspaceRecord assert WORKSPACES_HEADER == [ @@ -184,6 +181,8 @@ def test_workspaces_header_and_rows(): "num_projects", "num_experiments", "data_mb", + "mpm_enabled", + "num_monitored_models", ] ws = [ WorkspaceRecord( @@ -198,6 +197,53 @@ def test_workspaces_header_and_rows(): # data_mb renders as a plain decimal string (never scientific notation) assert rows[0][:5] == [DATE, "research", 2, 24, 2130] assert float(rows[0][5]) == 12422.75 + # MPM not reported by the server -> empty cells (NULL in Glue), not 0 + assert rows[0][6:] == ["", ""] + + +def test_workspaces_rows_mpm_columns(): + from cometx.cli.admin_growth_csv import build_workspaces_rows + from cometx.cli.admin_growth_users import WorkspaceRecord + + base = dict(num_experiments=0, data_mb=0.0, num_projects=0, members=()) + ws = [ + WorkspaceRecord(name="fraud", mpm_enabled=True, num_monitored_models=3, **base), + WorkspaceRecord( + name="research", mpm_enabled=False, num_monitored_models=0, **base + ), + WorkspaceRecord( + name="ops", mpm_enabled=True, num_monitored_models=None, **base + ), + ] + rows = build_workspaces_rows(ws, DATE) + # 1/0 so SUM(mpm_enabled) counts MPM workspaces + assert [r[6:] for r in rows] == [[1, 3], [0, 0], [1, ""]] + + +def test_collect_org_kpis_mpm_metrics(): + from cometx.cli.admin_growth_csv import collect_org_kpis + from cometx.cli.admin_growth_users import WorkspaceRecord + + base = dict(num_experiments=0, data_mb=0.0, num_projects=0, members=()) + common = dict(users=[], stats=None, growth=None, split=None, active_window_days=60) + + legacy = [WorkspaceRecord(name="w", **base)] + names = {k[0] for k in collect_org_kpis(ws_records=legacy, **common)} + assert "mpm_workspaces" not in names + assert "total_monitored_models" not in names + + ws = [ + WorkspaceRecord(name="fraud", mpm_enabled=True, num_monitored_models=3, **base), + WorkspaceRecord( + name="research", mpm_enabled=False, num_monitored_models=0, **base + ), + ] + by_name = { + name: (value, unit) + for name, value, unit, _t in collect_org_kpis(ws_records=ws, **common) + } + assert by_name["mpm_workspaces"] == (1, "count") + assert by_name["total_monitored_models"] == (3, "count") def test_org_kpi_rows_are_long_format(): diff --git a/tests/unit/test_admin_growth_mpm.py b/tests/unit/test_admin_growth_mpm.py new file mode 100644 index 0000000..803c9f6 --- /dev/null +++ b/tests/unit/test_admin_growth_mpm.py @@ -0,0 +1,202 @@ +from unittest.mock import MagicMock + +import pytest + + +def _api(member_payload=None, member_error=None, registry=None, details=None): + """A fake comet_ml API. + + `member_payload`: the mpm/v3/workspaces JSON (or `member_error` to raise). + `registry`: {workspace: [model names]} or an Exception per workspace. + `details`: {(workspace, model): details dict} or an Exception. + """ + api = MagicMock() + api.config = {"comet.url_override": "https://comet.example.com"} + api.api_key = "KEY" + + def get(url, headers=None, params=None): + assert url == "https://comet.example.com/api/mpm/v3/workspaces" + assert headers == {"Authorization": "KEY"} + if member_error is not None: + raise member_error + response = MagicMock() + response.json.return_value = member_payload or {"workspaces": []} + return response + + def get_registry_models(ws): + value = (registry or {}).get(ws, []) + if isinstance(value, Exception): + raise value + return [{"modelName": n, "registryModelId": "id-" + n} for n in value] + + def get_registry_model_details(ws, name): + value = (details or {})[(ws, name)] + if isinstance(value, Exception): + raise value + return value + + api._client.get.side_effect = get + api._client.get_registry_models.side_effect = get_registry_models + api._client.get_registry_model_details.side_effect = get_registry_model_details + return api + + +def test_member_workspaces_answered_from_one_call(): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api( + member_payload={ + "workspaces": [ + { + "workspaceName": "fraud", + "models": [{"modelId": "m1", "modelName": "scorer"}], + }, + {"workspaceName": "research", "models": []}, + # not in the requested set: ignored + {"workspaceName": "other", "models": [{"modelId": "x"}]}, + ] + } + ) + presence = fetch_mpm_presence(api, ["fraud", "research"]) + assert presence == {"fraud": [{"id": "m1", "name": "scorer"}], "research": []} + # every requested workspace was covered by v3: no registry calls + api._client.get_registry_models.assert_not_called() + + +def test_non_member_workspaces_fall_back_to_registry(): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api( + member_payload={"workspaces": [{"workspaceName": "fraud", "models": []}]}, + registry={"credit": ["pd", "lgd", "ead"]}, + details={ + ("credit", "pd"): {"isMonitored": True}, + # Jackson may drop the `is` prefix: accept `monitored` too + ("credit", "lgd"): {"monitored": True}, + ("credit", "ead"): {"isMonitored": False}, + }, + ) + presence = fetch_mpm_presence(api, ["fraud", "credit"]) + assert presence["fraud"] == [] + # sorted by name for stable output + assert presence["credit"] == [ + {"id": "id-lgd", "name": "lgd"}, + {"id": "id-pd", "name": "pd"}, + ] + + +def test_v3_failure_uses_registry_for_everything(): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api( + member_error=RuntimeError("404"), + registry={"a": ["m"], "b": []}, + details={("a", "m"): {"isMonitored": True}}, + ) + presence = fetch_mpm_presence(api, ["a", "b"]) + assert presence == {"a": [{"id": "id-m", "name": "m"}], "b": []} + + +@pytest.mark.parametrize( + "registry, details", + [ + # listing fails + ({"w": RuntimeError("403")}, {}), + # one detail lookup fails + ( + {"w": ["m1", "m2"]}, + {("w", "m1"): {"isMonitored": True}, ("w", "m2"): RuntimeError("500")}, + ), + # flag missing from the response + ({"w": ["m1"]}, {("w", "m1"): {"modelName": "m1"}}), + ], +) +def test_unknown_is_none_not_empty(registry, details): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api(registry=registry, details=details) + assert fetch_mpm_presence(api, ["w"]) == {"w": None} + + +def test_apply_mpm_presence_leaves_unknown_workspaces_untouched(): + from cometx.cli.admin_growth_mpm import apply_mpm_presence + from cometx.cli.admin_growth_users import parse_workspaces + + cb = { + "workspaces": [{"name": "a"}, {"name": "b"}, {"name": "c"}], + "users": {"licensedUsers": []}, + } + out = apply_mpm_presence( + cb, {"a": [{"id": "m1", "name": "scorer"}], "b": [], "c": None} + ) + assert cb["workspaces"][0] == {"name": "a"} # input not mutated + ws = {w.name: w for w in parse_workspaces(out)} + assert (ws["a"].mpm_enabled, ws["a"].num_monitored_models) == (True, 1) + assert ws["a"].monitored_models == ("scorer",) + assert (ws["b"].mpm_enabled, ws["b"].num_monitored_models) == (False, 0) + assert (ws["c"].mpm_enabled, ws["c"].num_monitored_models) == (None, None) + + +def _chargeback(): + return { + "workspaces": [ + {"name": "fraud", "members": [{"userName": "alice"}]}, + {"name": "credit", "members": [{"userName": "bob"}]}, + ], + "users": { + "licensedUsers": [ + {"username": "alice", "email": "a", "lastUsedAt": 1}, + {"username": "bob", "email": "b", "lastUsedAt": 1}, + ] + }, + } + + +def test_build_without_mpm_makes_no_mpm_calls(): + from cometx.cli.admin_growth_report import GrowthReporter + + api = _api() + r = GrowthReporter(api, window="7d", units="month") + r.build([], chargeback=_chargeback()) + api._client.get_registry_models.assert_not_called() + _users, ws_records, kpis = r.last_parsed() + assert all(w.mpm_enabled is None for w in ws_records) + assert "mpm_workspaces_unchecked" not in {k[0] for k in kpis} + + +def test_build_with_mpm_merges_presence_and_reports_unchecked(): + from cometx.cli.admin_growth_report import GrowthReporter + + api = _api( + member_payload={ + "workspaces": [ + { + "workspaceName": "fraud", + "models": [{"modelId": "m1", "modelName": "scorer"}], + } + ] + }, + registry={"credit": RuntimeError("403")}, + ) + r = GrowthReporter(api, window="7d", units="month", mpm=True) + report = r.build([], chargeback=_chargeback()) + _users, ws_records, kpis = r.last_parsed() + ws = {w.name: w for w in ws_records} + assert ws["fraud"].num_monitored_models == 1 + assert ws["credit"].mpm_enabled is None # unknown, not "no MPM" + by_name = {k[0]: k[1] for k in kpis} + assert by_name["mpm_workspaces"] == 1 + assert by_name["mpm_workspaces_unchecked"] == 1 + overview = report["sections"]["unified"] + models_kpi = next(k for k in overview["kpis"] if k["label"] == "Monitored models") + assert models_kpi["sub"] == "MPM checked in 1/2 workspaces" + + +def test_build_with_mpm_only_checks_scoped_workspaces(): + from cometx.cli.admin_growth_report import GrowthReporter + + api = _api(member_error=RuntimeError("404"), registry={"credit": []}) + r = GrowthReporter(api, window="7d", units="month", mpm=True) + r.build(["credit"], chargeback=_chargeback()) + called = [c.args[0] for c in api._client.get_registry_models.call_args_list] + assert called == ["credit"] diff --git a/tests/unit/test_admin_growth_report.py b/tests/unit/test_admin_growth_report.py index dda2a28..51a1f67 100644 --- a/tests/unit/test_admin_growth_report.py +++ b/tests/unit/test_admin_growth_report.py @@ -2300,3 +2300,88 @@ def test_html_scope_label_and_csv_scope_kpi_agree_on_exclude_personal(tmp_path): } assert rows["scope"][1] == "organization_excluding_personal" assert "excluding personal" in report_data["meta"]["scope"] + + +def _cb_mpm(with_mpm=True): + fraud = { + "name": "fraud", + "numberOfExperiments": 5, + "projects": [], + "members": [{"userName": "alice"}], + } + research = { + "name": "research", + "numberOfExperiments": 50, + "projects": [{}], + "members": [{"userName": "bob"}], + } + if with_mpm: + fraud.update( + mpmEnabled=True, + monitoredModels=[{"id": "m1", "name": "a"}, {"id": "m2", "name": "b"}], + ) + research.update(mpmEnabled=False, monitoredModels=[]) + return { + "workspaces": [fraud, research], + "users": { + "licensedUsers": [ + {"username": "alice", "email": "a", "lastUsedAt": 1}, + {"username": "bob", "email": "b", "lastUsedAt": 1}, + ] + }, + } + + +def _unified(cb): + from cometx.cli.admin_growth_report import GrowthReporter + from cometx.cli.admin_growth_users import parse_users, parse_workspaces + + r = GrowthReporter(MagicMock(), window="7d", units="month") + return r._build_unified_section( + _win(), + people_users=parse_users(cb), + ws_records=parse_workspaces(cb), + now_ms=2, + active_window_days=30, + ) + + +def test_unified_section_mpm_kpis_and_table_column(): + section = _unified(_cb_mpm()) + kpis = {k["label"]: k for k in section["kpis"]} + assert kpis["MPM workspaces"]["value"] == 1 + assert kpis["MPM workspaces"]["sub"] == "50% of workspaces" + assert kpis["Monitored models"]["value"] == 2 + table = section["table"] + assert table["headers"][-1] == "MPM models" + by_ws = {row[0]: row[-1] for row in table["rows"]} + assert by_ws == {"fraud": 2, "research": 0} + mix = next(c for c in section["charts"] if c["id"] == "chart-unified-platform-mix") + assert "MPM shown separately" in mix["hint"] + + +def test_unified_section_without_mpm_hides_mpm_and_says_so(): + section = _unified(_cb_mpm(with_mpm=False)) + labels = [k["label"] for k in section["kpis"]] + assert "MPM workspaces" not in labels + assert "Monitored models" not in labels + assert "MPM models" not in section["table"]["headers"] + mix = next(c for c in section["charts"] if c["id"] == "chart-unified-platform-mix") + assert "MPM not collected (run with --mpm)" in mix["hint"] + + +def test_leaderboards_mpm_models_only_when_reported(): + from cometx.cli.admin_growth_report import GrowthReporter + from cometx.cli.admin_growth_users import parse_users, parse_workspaces + + r = GrowthReporter(MagicMock(), window="7d", units="month", leaderboard_top_n=5) + for cb, expected in ((_cb_mpm(), True), (_cb_mpm(with_mpm=False), False)): + section = r._build_leaderboards_section( + users=parse_users(cb), ws_records=parse_workspaces(cb) + ) + charts = {c["id"]: c for c in (section or {}).get("charts", [])} + assert ("chart-lb-ws-mpm-models-top" in charts) is expected + if expected: + rows = charts["chart-lb-ws-mpm-models-top"]["data"]["rows"] + # research has 0 monitored models and is left out + assert rows == [{"label": "fraud", "value": 2}] diff --git a/tests/unit/test_admin_growth_users.py b/tests/unit/test_admin_growth_users.py index da5bc38..269f805 100644 --- a/tests/unit/test_admin_growth_users.py +++ b/tests/unit/test_admin_growth_users.py @@ -608,6 +608,9 @@ def test_parse_workspaces_org_totals_and_platform_mix(): "projects": 3, "experiments": 13, "data_mb": 5.0, + # no MPM fields in the payload -> not reported, not zero + "mpm_workspaces": None, + "monitored_models": None, } # em-ws=EM only; opik-ws=Opik only (b has spans); both-ws=both; empty-ws=neither assert platform_mix(ws, parse_users(cb)) == { @@ -747,3 +750,63 @@ def test_adoption_rate_series_omits_capability_without_signal(): keys = pts[0]["values"].keys() assert "overall" in keys and "em" in keys assert "opik" not in keys # no opik signal -> omitted, not a flat 0% + + +def test_parse_workspaces_mpm_absent_is_none_not_zero(): + from cometx.cli.admin_growth_users import mpm_reported, parse_workspaces + + ws = parse_workspaces({"workspaces": [{"name": "w", "members": []}]}) + assert ws[0].mpm_enabled is None + assert ws[0].num_monitored_models is None + assert ws[0].monitored_models == () + assert not mpm_reported(ws) + + +def test_parse_workspaces_mpm_fields(): + from cometx.cli.admin_growth_users import ( + mpm_reported, + parse_workspaces, + workspace_org_totals, + ) + + cb = { + "workspaces": [ + # explicit flag + list of model dicts + { + "name": "fraud", + "mpmEnabled": True, + "monitoredModels": [ + {"id": "m1", "name": "scorer"}, + {"id": "m2", "modelName": "ranker"}, + {"id": "m3"}, + ], + }, + # count only, flag inferred from it + {"name": "credit", "monitoredModels": 2}, + # list of plain names, flag inferred + {"name": "risk", "monitoredModels": ["a"]}, + # reported as not enabled, no models + {"name": "research", "mpmEnabled": False, "monitoredModels": []}, + # flag without a model list: enabled, count unknown + {"name": "ops", "mpmEnabled": True}, + # malformed values are ignored rather than trusted + {"name": "bad", "mpmEnabled": "yes", "monitoredModels": True}, + ] + } + ws = {w.name: w for w in parse_workspaces(cb)} + assert ws["fraud"].mpm_enabled is True + assert ws["fraud"].num_monitored_models == 3 + assert ws["fraud"].monitored_models == ("scorer", "ranker", "m3") + assert (ws["credit"].mpm_enabled, ws["credit"].num_monitored_models) == (True, 2) + assert ws["risk"].monitored_models == ("a",) + assert (ws["research"].mpm_enabled, ws["research"].num_monitored_models) == ( + False, + 0, + ) + assert (ws["ops"].mpm_enabled, ws["ops"].num_monitored_models) == (True, None) + assert (ws["bad"].mpm_enabled, ws["bad"].num_monitored_models) == (None, None) + assert mpm_reported(list(ws.values())) + + totals = workspace_org_totals(list(ws.values())) + assert totals["mpm_workspaces"] == 4 # fraud, credit, risk, ops + assert totals["monitored_models"] == 6 # 3 + 2 + 1 (+ ops unknown) From c69fd836df332b1b86ff3c430bcc9d2ccf7d661f Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Tue, 29 Sep 2026 11:27:38 -0700 Subject: [PATCH 2/9] fix(admin): call mpm/v3/workspaces beside /clientlib, not under it The SDK's comet.url_override ends in /clientlib/, and MPM is served next to it. Under it, the call 404'd and --mpm silently fell back to the slower per-model registry path. Found running against mpm-demo.dev.comet.com, where /clientlib/api/mpm/v3/workspaces is 404 and /api/mpm/v3/workspaces is 200. Co-Authored-By: Claude Opus 5.5 (1M context) --- cometx/cli/admin_growth_mpm.py | 13 ++++++++++++- tests/unit/test_admin_growth_mpm.py | 25 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py index 307a4e4..4a7df47 100644 --- a/cometx/cli/admin_growth_mpm.py +++ b/cometx/cli/admin_growth_mpm.py @@ -45,13 +45,24 @@ DEFAULT_MAX_WORKERS = 8 +def _mpm_url(api) -> str: + """URL of `mpm/v3/workspaces`. The SDK's `comet.url_override` normally + ends in `/clientlib/` (the SDK's own API root); MPM is served beside it, + not under it, so that last segment is dropped. Any deployment prefix in + front of it (e.g. `/comet/clientlib/`) is kept.""" + base = api.config["comet.url_override"].rstrip("/") + if base.endswith("/clientlib"): + base = base[: -len("/clientlib")] + return admin_api_url(base, MPM_WORKSPACES_PATH) + + def _fetch_member_workspaces(api) -> "dict[str, list[dict]] | None": """`mpm/v3/workspaces` -> {workspace_name: [{"id", "name"}, ...]} for the workspaces the caller belongs to. `None` on any failure (MPM disabled on the deployment, network error, unexpected shape): the registry path then covers every workspace instead.""" try: - url = admin_api_url(api.config["comet.url_override"], MPM_WORKSPACES_PATH) + url = _mpm_url(api) response = api._client.get( url, headers={"Authorization": api.api_key}, params={} ) diff --git a/tests/unit/test_admin_growth_mpm.py b/tests/unit/test_admin_growth_mpm.py index 803c9f6..75f4603 100644 --- a/tests/unit/test_admin_growth_mpm.py +++ b/tests/unit/test_admin_growth_mpm.py @@ -200,3 +200,28 @@ def test_build_with_mpm_only_checks_scoped_workspaces(): r.build(["credit"], chargeback=_chargeback()) called = [c.args[0] for c in api._client.get_registry_models.call_args_list] assert called == ["credit"] + + +@pytest.mark.parametrize( + "override, expected", + [ + ( + "https://comet.example.com/clientlib/", + "https://comet.example.com/api/mpm/v3/workspaces", + ), + ( + "https://comet.example.com/comet/clientlib", + "https://comet.example.com/comet/api/mpm/v3/workspaces", + ), + ( + "https://comet.example.com", + "https://comet.example.com/api/mpm/v3/workspaces", + ), + ], +) +def test_mpm_url_drops_sdk_clientlib_segment(override, expected): + from cometx.cli.admin_growth_mpm import _mpm_url + + api = MagicMock() + api.config = {"comet.url_override": override} + assert _mpm_url(api) == expected From 6e30a61af3c9bd052d46ab72767affdb3641cea8 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Tue, 29 Sep 2026 11:40:55 -0700 Subject: [PATCH 3/9] docs(admin): --mpm needs an organization admin to be complete For a workspace the user isn't a member of, the registry returns only public models to a user who isn't an organization admin, with no error. Private monitored models are then missed silently and the workspace still counts as checked. This rule differs from chargeback's (server admin list, or org admin on-prem), so a key that passes chargeback may still undercount. Co-Authored-By: Claude Opus 5.5 (1M context) --- README-ADMIN.md | 4 ++-- README.md | 2 +- cometx/cli/admin.py | 7 ++++--- cometx/cli/admin_growth_mpm.py | 6 ++++++ 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/README-ADMIN.md b/README-ADMIN.md index fc367ee..42fa21d 100644 --- a/README-ADMIN.md +++ b/README-ADMIN.md @@ -172,7 +172,7 @@ report is scoped to just those workspaces. - **`--no-open`**: Don't automatically open the generated HTML file after generation. - **`--csv-dir DIR`**: Also write Glue-ready CSV fact tables (`growth_users.csv`, `growth_workspaces.csv`, `growth_org_kpis.csv`) into `DIR`. `DIR` is created if it doesn't exist. - **`--no-html`**: Skip the HTML report. Requires `--csv-dir` (otherwise there is nothing to write, and the command errors out). -- **`--mpm`**: Include MPM presence — whether each workspace uses MPM and how many models it monitors (registry models flagged `is_monitored`). Chargeback has no MPM data, so this is collected from `/api/mpm/v3/workspaces` (one call, covering the workspaces the API key's user belongs to) and, for the rest, the REST v2 model registry (one request per registry model). Use an org-admin key so every workspace is visible. Off by default because of the extra requests. +- **`--mpm`**: Include MPM presence — whether each workspace uses MPM and how many models it monitors (registry models flagged `is_monitored`). Chargeback has no MPM data, so this is collected from `/api/mpm/v3/workspaces` (one call, covering the workspaces the API key's user belongs to) and, for the rest, the REST v2 model registry (one request per registry model). The key's user must be an **organization admin** for this to be complete. That's a different check from the one chargeback applies: the chargeback endpoint accepts server admins, while the registry lets organization admins read every workspace. For a workspace the user isn't a member of, a key that isn't an organization admin gets only the workspace's *public* models, so private monitored models are silently missed and the workspace still counts as checked. Off by default because of the extra requests. - **`--chargeback-report FILE`**: Read the chargeback report from a local JSON file (as saved by `cometx admin chargeback-report`) instead of calling the chargeback endpoint. Useful for CSV-only pipelines that already have a saved snapshot, or for re-running without re-fetching it. Note this skips the *chargeback* request only — the report still queries `/api/admin/service-accounts` to classify accounts, and falls back to a name-pattern heuristic if that request fails (`service_account_source` records which was used). ### The two time concepts @@ -220,7 +220,7 @@ The report generates a single self-contained HTML file containing: - **Chargeback is required.** The whole report is derived from the admin chargeback report; without admin access the command errors out (non-zero exit). - **Workspace "created" is a proxy** — the earliest member `createdAt` in that workspace, since chargeback has no workspace-creation timestamp. The added-vs-deleted "deleted" series is also a best-effort proxy (all members removed) and typically reads ~0. - **"Total projects" counts EM projects only** — chargeback's per-workspace `projects[]` covers Experiment Management. Opik projects and MPM aren't represented there (Opik appears only as a per-user span count; MPM is absent), so the platform mix uses an Opik per-user proxy and excludes MPM. -- **MPM comes from `--mpm`, not chargeback.** It reflects which models are monitored *now*, so past months can't be reconstructed; a trend builds up from monthly `--csv-dir` exports. A workspace whose MPM lookup fails is reported as unknown (empty in the CSV), never as zero, and the report says how many workspaces were checked. `nb_models_registered` from the monthly usage report is not used: it counts every registry model and overstates MPM adoption. +- **MPM comes from `--mpm`, not chargeback.** It reflects which models are monitored *now*, so past months can't be reconstructed; a trend builds up from monthly `--csv-dir` exports. A workspace whose MPM lookup fails is reported as unknown (empty in the CSV), never as zero, and the report says how many workspaces were checked. Counts are only complete when the key's user is an organization admin or a member of every workspace (see `--mpm`). `nb_models_registered` from the monthly usage report is not used: it counts every registry model and overstates MPM adoption. - **The people layer degrades independently.** If the chargeback payload parses but a section's inputs are missing, a warning is printed and only that section is dropped — the rest of the report still generates. ### CSV export diff --git a/README.md b/README.md index 5c075a1..0ecc200 100644 --- a/README.md +++ b/README.md @@ -605,7 +605,7 @@ cometx admin growth-report [WORKSPACE ...] * `--exclude-personal` - Drop workspaces whose name matches `--personal-pattern` from the chargeback data (default: off; no effect without `--personal-pattern`). * `--personal-pattern REGEX` - Regex used with `--exclude-personal` to identify personal-workspace names to drop, e.g. `'^user-'` (default: none). * `--no-open` - Don't automatically open the generated HTML file after generation. -* `--mpm` - Include MPM presence (monitored models per workspace), collected from the MPM and model-registry APIs. Use an org-admin API key; off by default because it makes one request per registry model in workspaces the key's user isn't a member of. +* `--mpm` - Include MPM presence (monitored models per workspace), collected from the MPM and model-registry APIs. The key's user must be an organization admin (a separate check from chargeback's server-admin one) or private models in workspaces they aren't a member of are silently missed; off by default because it makes one request per registry model in workspaces the key's user isn't a member of. **Two time concepts:** * `--units` controls chart *granularity* — every chart shows the full all-time history bucketed at this resolution. diff --git a/cometx/cli/admin.py b/cometx/cli/admin.py index 178a79f..d4087b6 100644 --- a/cometx/cli/admin.py +++ b/cometx/cli/admin.py @@ -590,9 +590,10 @@ def get_parser_arguments(parser): action="store_true", help=( "Include MPM presence (monitored models per workspace), collected " - "from the MPM and model-registry APIs. Needs an org-admin API key " - "to see every workspace; makes one request per registry model in " - "workspaces the key's user is not a member of" + "from the MPM and model-registry APIs. The key's user must be an " + "organization admin (or a member of every workspace); otherwise " + "private models in other workspaces are silently missed. Makes one " + "request per registry model in workspaces the user is not a member of" ), ) growth_parser.add_argument( diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py index 4a7df47..30eba5a 100644 --- a/cometx/cli/admin_growth_mpm.py +++ b/cometx/cli/admin_growth_mpm.py @@ -29,6 +29,12 @@ for its monitored flag. Org admins may read any workspace this way, private models included. One request per model, so it runs in a pool. + Access caveat: for a non-member workspace, a user who is NOT an org admin + gets only its public models, with no error -- so private monitored models + are missed and the workspace still reads as checked. This is a different + rule from chargeback's (server admin list, or org admin on-prem), so a key + that passes chargeback is not necessarily complete here. + A workspace whose lookup fails is reported as unknown (`None`), never as "no MPM": a false zero would read as a workspace that stopped using MPM. `nb_models_registered` (the monthly usage report) is deliberately NOT used -- From e0037a48d50876388db6342f63f65bc78d22f234 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Tue, 29 Sep 2026 11:40:55 -0700 Subject: [PATCH 4/9] fix(admin): accurate growth-report errors when chargeback fetch fails Every chargeback failure used to be reported as "requires an admin API key", including a 404 from a wrong URL or path prefix. Now: - 401/403: needs an admin user's key (a server admin, or an organization admin on self-hosted); workspace roles such as Manage don't count. - 404: endpoint not found at ; check the server URL. - non-JSON response: likely an SSO or proxy login page. - other failures: could not fetch, without blaming the key. Show "HTTP : " instead of the SDK exception text, which included part of the API key and a truncated URL. Document who counts as an admin in both READMEs. Co-Authored-By: Claude Opus 5.5 (1M context) --- README-ADMIN.md | 10 +++-- README.md | 6 ++- cometx/cli/admin_growth_report.py | 62 +++++++++++++++++++++++--- tests/unit/test_admin_growth_report.py | 62 ++++++++++++++++++++++++-- 4 files changed, 127 insertions(+), 13 deletions(-) diff --git a/README-ADMIN.md b/README-ADMIN.md index 42fa21d..5206426 100644 --- a/README-ADMIN.md +++ b/README-ADMIN.md @@ -144,9 +144,13 @@ HTML page, built entirely from the **admin chargeback report**. Distinct from `usage-report` (an experiment-count PDF): `growth-report` gives an org-wide view of workspaces, users, and platform adoption, broken down by workspace/department. -**Requires an admin API key.** The report is derived entirely from the admin -chargeback endpoint; with a non-admin key the command prints an error and exits -non-zero — there is no fallback. +**Requires an admin user's API key.** The report is derived entirely from the +admin chargeback endpoint, which accepts only server admins (users in the +server's admin user list) and, on self-hosted installs, organization admins of +the install's organization. Workspace roles such as Manage don't count. With any +other key the command prints an error and exits non-zero — there is no fallback. +If the server restricts admin calls to localhost, even an admin key is refused +("invalid connection") from anywhere else. ### Basic Usage diff --git a/README.md b/README.md index 0ecc200..4aec9da 100644 --- a/README.md +++ b/README.md @@ -585,8 +585,10 @@ page, built entirely from the **admin chargeback report**. Distinct from `usage-report` (experiment-count PDF): `growth-report` gives an org-wide view of workspaces, users, and platform adoption, per workspace/department. -**Requires an admin API key** — the report is derived entirely from the admin -chargeback endpoint, and with a non-admin key the command errors out (non-zero +**Requires an admin user's API key** — the report is derived entirely from the +admin chargeback endpoint, which accepts only server admins (the server's admin +user list) and, on self-hosted installs, organization admins. Workspace roles +such as Manage don't count. With any other key the command errors out (non-zero exit). ``` diff --git a/cometx/cli/admin_growth_report.py b/cometx/cli/admin_growth_report.py index 7a1fb31..faead25 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -24,6 +24,7 @@ import dataclasses import datetime +import json import os import re @@ -210,6 +211,61 @@ def _fetch_service_accounts(api) -> "set[str] | None": return None +def _error_status(exc) -> "int | None": + """HTTP status of a failed request: from the SDK exception's `response` + when it has one, else parsed from its text (`status_code: NNN`).""" + status = getattr(getattr(exc, "response", None), "status_code", None) + if isinstance(status, int): + return status + match = re.search(r"status_code:\s*(\d+)", exception_text(exc)) + return int(match.group(1)) if match else None + + +def _chargeback_error_message(exc) -> str: + """Say what actually went wrong fetching the chargeback report, rather + than attributing every failure to a non-admin key: a 404 (wrong URL or + path prefix) or a 5xx says nothing about the key.""" + status = _error_status(exc) + response = getattr(exc, "response", None) + url = getattr(response, "url", None) + # Prefer "HTTP 401: " over the SDK exception's text, which + # carries a partly-masked API key and a truncated URL. + detail = _short_api_error(exc) + if isinstance(status, int) and response is not None: + try: + server_msg = response.json().get("msg") + except Exception: + server_msg = None + detail = "HTTP %d" % status + ( + ": %s" % server_msg if isinstance(server_msg, str) and server_msg else "" + ) + at = " at %s" % redact_url_userinfo(url) if isinstance(url, str) else "" + if status in (401, 403): + return ( + "growth-report requires an admin user's API key: the chargeback " + "endpoint refused this key (%s). The key's user must be a server " + "admin, or an organization admin on self-hosted installs; " + "workspace roles such as Manage do not count. This report is " + "built entirely from the admin chargeback report." % detail + ) + if status == 404: + return ( + "growth-report could not find the chargeback endpoint%s (%s). " + "Check the server URL (COMET_URL_OVERRIDE / comet.url_override); " + "this is not an API-key problem." % (at, detail) + ) + if isinstance(exc, json.JSONDecodeError): + return ( + "growth-report got a non-JSON response from the chargeback " + "endpoint%s -- often an SSO or proxy login page. Check the server " + "URL, and that the API key belongs to an admin." % at + ) + return "growth-report could not fetch the chargeback report%s (%s)." % ( + at, + detail, + ) + + def _short_api_error(exc): """Condense a verbose SDK/HTTP exception (which may dump full response headers, cookies, and CSP) into a single short line: "status: message" @@ -453,11 +509,7 @@ def build(self, workspaces, chargeback=None): "%s" % exception_text(exc) ) from exc except Exception as exc: - raise GrowthReportError( - "growth-report requires an admin API key: the chargeback " - f"endpoint is unavailable ({_short_api_error(exc)}). This " - "report is built entirely from the admin chargeback report." - ) from exc + raise GrowthReportError(_chargeback_error_message(exc)) from exc # Structural check AFTER the fetch, so it covers both sources with one # implementation. The chargeback parsers are deliberately permissive, # returning empty lists rather than raising, so without this a report diff --git a/tests/unit/test_admin_growth_report.py b/tests/unit/test_admin_growth_report.py index 51a1f67..33b6564 100644 --- a/tests/unit/test_admin_growth_report.py +++ b/tests/unit/test_admin_growth_report.py @@ -1112,7 +1112,7 @@ def boom(*a, **k): r.build([]) assert False, "expected GrowthReportError" except GrowthReportError as exc: - assert "admin API key" in str(exc) + assert "admin user's API key" in str(exc) def test_build_reports_malformed_url_as_a_url_problem(monkeypatch): @@ -1128,7 +1128,7 @@ def boom(*a, **k): with pytest.raises(GrowthReportError) as exc_info: r.build([]) assert "could not reach the chargeback endpoint" in str(exc_info.value) - assert "admin API key" not in str(exc_info.value) + assert "requires an admin" not in str(exc_info.value) def test_build_reports_non_json_response_as_unavailable_not_bad_url(monkeypatch): @@ -1147,10 +1147,66 @@ def boom(*a, **k): r = GrowthReporter(MagicMock(), window="7d", units="month") with pytest.raises(GrowthReportError) as exc_info: r.build([]) - assert "admin API key" in str(exc_info.value) + assert "non-JSON response" in str(exc_info.value) assert "could not reach the chargeback endpoint" not in str(exc_info.value) +def _http_error(status, url="https://c.example.com/api/admin/chargeback/report"): + exc = RuntimeError("GET failed with status code %d" % status) + exc.response = MagicMock(status_code=status, url=url) + return exc + + +def _build_error(monkeypatch, exc): + import cometx.cli.admin_growth_report as agr + from cometx.cli.admin_growth_report import GrowthReporter, GrowthReportError + + def boom(*a, **k): + raise exc + + monkeypatch.setattr(agr, "fetch_chargeback_report", boom) + r = GrowthReporter(MagicMock(), window="7d", units="month") + with pytest.raises(GrowthReportError) as exc_info: + r.build([]) + return str(exc_info.value) + + +@pytest.mark.parametrize("status", [401, 403]) +def test_chargeback_auth_failure_says_admin_key(monkeypatch, status): + exc = _http_error(status) + exc.response.json.return_value = {"msg": "You don't have permission"} + msg = _build_error(monkeypatch, exc) + assert "requires an admin user's API key" in msg + assert "refused this key (HTTP %d: You don't have permission)" % status in msg + assert "workspace roles such as Manage do not count" in msg + + +def test_chargeback_404_is_a_url_problem_not_a_key_problem(monkeypatch): + msg = _build_error( + monkeypatch, + _http_error(404, "https://u:p@c.example.com/clientlib/api/admin/x"), + ) + assert "could not find the chargeback endpoint" in msg + assert "requires an admin" not in msg + assert "COMET_URL_OVERRIDE" in msg + # the URL is shown, with credentials redacted + assert "c.example.com/clientlib/api/admin/x" in msg + assert "u:p@" not in msg + + +def test_chargeback_server_error_does_not_blame_the_key(monkeypatch): + msg = _build_error(monkeypatch, _http_error(502)) + assert "could not fetch the chargeback report" in msg + assert "requires an admin" not in msg + + +def test_chargeback_status_parsed_from_text_when_no_response(): + from cometx.cli.admin_growth_report import _chargeback_error_message + + exc = RuntimeError("status_code: 403, body: {'message': 'Forbidden'}") + assert "requires an admin user's API key" in _chargeback_error_message(exc) + + def test_generate_growth_report_signature_has_no_sdk_kwargs(): import inspect From 25da13f9998655f6cd5d3282016d23e2cdb7d285 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Wed, 30 Sep 2026 07:36:41 -0700 Subject: [PATCH 5/9] fix(admin): call admin APIs beside the SDK's /clientlib root, not under it admin_api_url kept every path prefix of the base URL, including the /clientlib segment the SDK's comet.url_override ends with. That segment is the SDK's own API root, and the admin and MPM APIs are served beside it, so chargeback, service-accounts and migrate-users called /clientlib/api/admin/... and got a 404 on servers such as mpm-demo, where /api/admin/... answers. Drop a trailing /clientlib segment and keep any real deployment prefix in front of it (/comet/clientlib -> /comet), the way smoke_test already does. The MPM-only workaround in admin_growth_mpm now just uses the shared helper. The old test that asserted the /clientlib/api/admin URL is replaced by cases for the SDK base, a deployment prefix, a prefix-only base, and a lookalike segment. Co-Authored-By: Claude Opus 5.5 (1M context) --- cometx/cli/admin_growth_mpm.py | 12 ++++------ cometx/utils.py | 12 ++++++++-- tests/unit/test_utils_chargeback.py | 34 ++++++++++++++++++++++++----- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py index 30eba5a..eaf9d07 100644 --- a/cometx/cli/admin_growth_mpm.py +++ b/cometx/cli/admin_growth_mpm.py @@ -52,14 +52,10 @@ def _mpm_url(api) -> str: - """URL of `mpm/v3/workspaces`. The SDK's `comet.url_override` normally - ends in `/clientlib/` (the SDK's own API root); MPM is served beside it, - not under it, so that last segment is dropped. Any deployment prefix in - front of it (e.g. `/comet/clientlib/`) is kept.""" - base = api.config["comet.url_override"].rstrip("/") - if base.endswith("/clientlib"): - base = base[: -len("/clientlib")] - return admin_api_url(base, MPM_WORKSPACES_PATH) + """URL of `mpm/v3/workspaces`, beside (not under) the SDK's `/clientlib` + root -- `admin_api_url` drops that segment and keeps any deployment + prefix.""" + return admin_api_url(api.config["comet.url_override"], MPM_WORKSPACES_PATH) def _fetch_member_workspaces(api) -> "dict[str, list[dict]] | None": diff --git a/cometx/utils.py b/cometx/utils.py index 02d92c2..a638a57 100644 --- a/cometx/utils.py +++ b/cometx/utils.py @@ -491,11 +491,19 @@ def admin_api_url(base, path): """Join an operator-supplied server base with an admin API `path`. Validates `base` via the shared `validate_server_base`, then preserves its - scheme, host, AND any path prefix (e.g. `/clientlib`) that on-prem - deployments sit behind. + scheme, host, and any path prefix a deployment sits behind (e.g. + `/comet`). + + A trailing `/clientlib` segment is dropped: it is the SDK's own API root + (the SDK's `comet.url_override` ends in `/clientlib/`), and the admin and + MPM APIs are served beside it, not under it. Joined naively, + `https://host/clientlib/` gave `https://host/clientlib/api/admin/...`, + which 404s. `smoke_test` strips it the same way. """ parsed = validate_server_base(base) prefix = parsed.path.rstrip("/") + if prefix == "/clientlib" or prefix.endswith("/clientlib"): + prefix = prefix[: -len("/clientlib")] return "%s://%s%s%s" % (parsed.scheme, parsed.netloc, prefix, path) diff --git a/tests/unit/test_utils_chargeback.py b/tests/unit/test_utils_chargeback.py index 3369cdc..7b23d80 100644 --- a/tests/unit/test_utils_chargeback.py +++ b/tests/unit/test_utils_chargeback.py @@ -57,13 +57,37 @@ def test_fetch_chargeback_allows_http_on_prem_base(): assert called_url == "http://comet.internal.corp/api/admin/chargeback/report" -def test_fetch_chargeback_preserves_path_prefix(): - # A base with a path prefix (e.g. /clientlib) must keep that prefix - # rather than silently dropping it. +@pytest.mark.parametrize( + "override, expected", + [ + # The SDK's url_override ends in /clientlib/: the admin API is served + # beside it, not under it (/clientlib/api/admin/... 404s). + ( + "https://comet.x.com/clientlib/", + "https://comet.x.com/api/admin/chargeback/report", + ), + # A real deployment prefix in front of /clientlib is kept. + ( + "https://comet.x.com/comet/clientlib/", + "https://comet.x.com/comet/api/admin/chargeback/report", + ), + # ...and a prefix without /clientlib is kept as-is. + ( + "https://comet.x.com/comet/", + "https://comet.x.com/comet/api/admin/chargeback/report", + ), + # Only a whole trailing segment is dropped, not a lookalike. + ( + "https://comet.x.com/myclientlib/", + "https://comet.x.com/myclientlib/api/admin/chargeback/report", + ), + ], +) +def test_fetch_chargeback_url_prefix_handling(override, expected): from cometx.utils import fetch_chargeback_report api = MagicMock() - api.config = {"comet.url_override": "https://comet.x.com/clientlib/"} + api.config = {"comet.url_override": override} api.api_key = "KEY" resp = MagicMock(status_code=200) resp.json.return_value = {} @@ -72,7 +96,7 @@ def test_fetch_chargeback_preserves_path_prefix(): fetch_chargeback_report(api) called_url = api._client.get.call_args[0][0] - assert called_url == ("https://comet.x.com/clientlib/api/admin/chargeback/report") + assert called_url == expected @pytest.mark.parametrize( From 835e57f1fc311a6bf30065cae09ce5434c3138b8 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Wed, 30 Sep 2026 07:42:43 -0700 Subject: [PATCH 6/9] fix(admin): label partial MPM results instead of presenting them as complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review comments on #28: - MPM workspaces card: the percentage is over the workspaces actually checked ("100% of 1 checked; 1 unknown"), not over all workspaces. - Monitored models: a total missing any workspace is shown as a lower bound ("≥ N"). workspace_org_totals now returns mpm_checked and mpm_unchecked, derived from the records, so partial coverage is labeled the same way for --chargeback-report files. The separate _mpm_status run state is removed. - mpm_workspaces_unchecked counts workspaces with an unknown model count, and is emitted even when every lookup failed. - apply_mpm_presence removes existing mpmEnabled / monitoredModels from a workspace whose lookup failed, instead of passing stale values through. - A fractional or negative monitoredModels count is treated as unknown instead of being truncated. - README-ADMIN: fix a typo, and say the CSV totals are exact only when mpm_workspaces_unchecked is 0. Co-Authored-By: Claude Opus 5.5 (1M context) --- README-ADMIN.md | 2 +- cometx/cli/admin_growth_csv.py | 21 ++++++------- cometx/cli/admin_growth_mpm.py | 14 +++++++-- cometx/cli/admin_growth_report.py | 45 ++++++++++++++++----------- cometx/cli/admin_growth_users.py | 18 ++++++++--- tests/unit/test_admin_growth_mpm.py | 36 ++++++++++++++++++++- tests/unit/test_admin_growth_users.py | 39 +++++++++++++++++++++++ 7 files changed, 134 insertions(+), 41 deletions(-) diff --git a/README-ADMIN.md b/README-ADMIN.md index 5206426..45c2382 100644 --- a/README-ADMIN.md +++ b/README-ADMIN.md @@ -257,7 +257,7 @@ Use the `deleted_users` KPI to see how many were excluded. **`growth_workspaces.csv`**: `report_date, workspace, member_count, num_projects, num_experiments, data_mb, mpm_enabled, num_monitored_models` -`mpm_enabled` is `1`/`0` (so `SUM(mpm_enabled)` counts MPM workspaces) and `num_monitored_models` a count; both are empty unless `--mpm` was given and that workspace could be checked. With `--mpm`, `growth_org_kpis.csv` also gets `mpm_workspaces`, `total_monitored_models`, and `mpm_workspaces_unchecked`. +`mpm_enabled` is `1`/`0` (so `SUM(mpm_enabled)` counts MPM workspaces) and `num_monitored_models` is a count; both are empty unless `--mpm` was given and that workspace could be checked. With `--mpm`, `growth_org_kpis.csv` also gets `mpm_workspaces`, `total_monitored_models`, and `mpm_workspaces_unchecked`. The two totals cover only the workspaces that could be checked, so they are exact only when `mpm_workspaces_unchecked` is 0; otherwise they are lower bounds, and the HTML report labels them that way. **`growth_org_kpis.csv`**: `report_date, metric_name, metric_value, metric_unit, metric_text` — long format so new metrics arrive as new rows without ever changing the Glue schema. `metric_unit` is one of `count`, `percent`, `megabytes`, `label`. diff --git a/cometx/cli/admin_growth_csv.py b/cometx/cli/admin_growth_csv.py index a06ca20..d05425b 100644 --- a/cometx/cli/admin_growth_csv.py +++ b/cometx/cli/admin_growth_csv.py @@ -247,7 +247,7 @@ def collect_org_kpis( active_window_days, scope=None, excluded_personal_count=0, - mpm_status=None, + mpm_requested=False, ): """Flatten the report's org-level numbers into (name, value, unit) triples. @@ -350,21 +350,18 @@ def collect_org_kpis( ("total_experiments", sum(w.num_experiments for w in ws_records), "count") ) kpis.append(("total_data_mb", sum(w.data_mb for w in ws_records), "megabytes")) - # MPM presence (--mpm). Omitted (not zero) when not collected. - # `mpm_workspaces_unchecked` says how many workspaces the totals could not - # cover, so a partial run is never mistaken for a complete one. + # MPM presence (--mpm). Omitted (not zero) when not collected. The totals + # cover the known workspaces only; `mpm_workspaces_unchecked` (workspaces + # whose model count is unknown) says how many they miss, so a partial run + # is never mistaken for a complete one -- the totals are exact only when + # it is 0. Emitted whenever MPM was requested or reported, including a + # run where every lookup failed. org = workspace_org_totals(ws_records) if org["mpm_workspaces"] is not None: kpis.append(("mpm_workspaces", org["mpm_workspaces"], "count")) kpis.append(("total_monitored_models", org["monitored_models"], "count")) - if mpm_status is not None: - kpis.append( - ( - "mpm_workspaces_unchecked", - mpm_status["total"] - mpm_status["checked"], - "count", - ) - ) + if mpm_requested or org["mpm_workspaces"] is not None: + kpis.append(("mpm_workspaces_unchecked", org["mpm_unchecked"], "count")) if split is not None: for bucket in ("personal", "service"): diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py index eaf9d07..f44448d 100644 --- a/cometx/cli/admin_growth_mpm.py +++ b/cometx/cli/admin_growth_mpm.py @@ -181,13 +181,21 @@ def fetch_mpm_presence( def apply_mpm_presence(chargeback, presence) -> dict: """Return a copy of `chargeback` whose workspaces carry `mpmEnabled` / `monitoredModels` from `presence`. Workspaces that are unknown (`None`) - or absent from `presence` are left without the fields, which - `parse_workspaces` reads as "not reported".""" + or absent from `presence` end up WITHOUT the fields -- any already in the + input (e.g. a hand-edited `--chargeback-report` file) are removed rather + than passed through as current -- which `parse_workspaces` reads as + "not reported".""" workspaces = [] for w in chargeback.get("workspaces") or []: models = presence.get(w.get("name")) if models is None: - workspaces.append(w) + workspaces.append( + { + k: v + for k, v in w.items() + if k not in ("mpmEnabled", "monitoredModels") + } + ) else: workspaces.append( {**w, "mpmEnabled": bool(models), "monitoredModels": list(models)} diff --git a/cometx/cli/admin_growth_report.py b/cometx/cli/admin_growth_report.py index faead25..83608f1 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -463,8 +463,6 @@ def __init__( self.api = api # --mpm: collect MPM presence client-side (see admin_growth_mpm). self.mpm = mpm - # {"checked": n, "total": m} after an --mpm build, else None. - self._mpm_status = None self.window = window self.units = units self.active_window = active_window @@ -540,7 +538,6 @@ def build(self, workspaces, chargeback=None): chargeback = self._filter_personal_chargeback(chargeback) excluded_personal_count = before - len((chargeback.get("workspaces") or [])) scope = set(workspaces) if workspaces else None - self._mpm_status = None if self.mpm: chargeback = self._add_mpm_presence(chargeback, scope) print("Building report...") @@ -566,8 +563,9 @@ def build(self, workspaces, chargeback=None): def _add_mpm_presence(self, chargeback, scope): """Merge client-side MPM presence into the chargeback workspaces (only - those in `scope`, when given, to avoid needless per-model calls), and - record how many could actually be checked.""" + those in `scope`, when given, to avoid needless per-model calls). + Unknown workspaces stay unknown in the records, which is what the + report and CSV count -- no separate run status is kept.""" names = [ w.get("name") for w in (chargeback.get("workspaces") or []) @@ -576,7 +574,6 @@ def _add_mpm_presence(self, chargeback, scope): print("Collecting MPM presence for %d workspace(s)..." % len(names)) presence = fetch_mpm_presence(self.api, names) checked = sum(1 for n in names if presence.get(n) is not None) - self._mpm_status = {"checked": checked, "total": len(names)} if checked < len(names): print( "Warning: could not determine MPM presence for %d of %d " @@ -593,11 +590,11 @@ def _mpm_note(self, ws_records): if not self.mpm else "MPM could not be determined" ) - status = self._mpm_status - if status and status["checked"] < status["total"]: + org = workspace_org_totals(ws_records) + if org["mpm_unchecked"]: return "MPM checked in %d/%d workspaces" % ( - status["checked"], - status["total"], + org["workspaces"] - org["mpm_unchecked"], + org["workspaces"], ) return "MPM shown separately" @@ -945,25 +942,35 @@ def _build_unified_section( # MPM presence (OPIK-8411, --mpm): registry models flagged # `is_monitored`. Only shown when collected -- rendering 0 otherwise # would read as "no MPM adoption". + # Partial coverage is labeled, never presented as complete: the + # percentage is over the workspaces actually checked, and a model total + # missing any workspace is shown as a lower bound. if org["mpm_workspaces"] is not None: - total_ws = org["workspaces"] - pct = round(100 * org["mpm_workspaces"] / total_ws) if total_ws else 0 + checked = org["mpm_checked"] + pct = round(100 * org["mpm_workspaces"] / checked) if checked else 0 + unknown_flags = org["workspaces"] - checked kpis.append( { "label": "MPM workspaces", "value": org["mpm_workspaces"], - "sub": f"{pct}% of workspaces", + "sub": ( + f"{pct}% of {checked} checked; {unknown_flags} unknown" + if unknown_flags + else f"{pct}% of workspaces" + ), } ) + partial = org["mpm_unchecked"] > 0 kpis.append( { "label": "Monitored models", - "value": org["monitored_models"], + "value": ( + f"≥ {org['monitored_models']}" + if partial + else org["monitored_models"] + ), "sub": ( - self._mpm_note(ws_records) - if self._mpm_status - and self._mpm_status["checked"] < self._mpm_status["total"] - else "MPM (is_monitored)" + self._mpm_note(ws_records) if partial else "MPM (is_monitored)" ), } ) @@ -1597,7 +1604,7 @@ def _assemble_report_data( active_window_days, scope=scope, excluded_personal_count=excluded_personal_count, - mpm_status=self._mpm_status, + mpm_requested=self.mpm, ), ) except Exception as exc: diff --git a/cometx/cli/admin_growth_users.py b/cometx/cli/admin_growth_users.py index c5766a2..9c20350 100644 --- a/cometx/cli/admin_growth_users.py +++ b/cometx/cli/admin_growth_users.py @@ -732,7 +732,8 @@ def _parse_mpm(w: dict) -> "tuple[bool | None, int | None, tuple]": """Read a chargeback workspace's MPM presence fields. `monitoredModels` may be a list (of model dicts or names; its length is - the count) or already a number. `mpmEnabled` is taken as given when it is + the count) or already a whole number; a fractional or negative number is + malformed and leaves the count unknown rather than truncating it. `mpmEnabled` is taken as given when it is a real bool, otherwise inferred from the count. Returns (mpm_enabled, num_monitored_models, monitored_model_names); all-`None` when the server reports neither field.""" @@ -753,7 +754,9 @@ def _parse_mpm(w: dict) -> "tuple[bool | None, int | None, tuple]": ) if n ) - elif isinstance(raw, (int, float)) and not isinstance(raw, bool): + elif isinstance(raw, int) and not isinstance(raw, bool): + count = raw if raw >= 0 else None + elif isinstance(raw, float) and raw.is_integer() and raw >= 0: count = int(raw) enabled = w.get("mpmEnabled") if not isinstance(enabled, bool): @@ -805,9 +808,12 @@ def workspace_org_totals(workspaces: "list[WorkspaceRecord]") -> dict: """Org-wide totals across all workspaces (from chargeback). `mpm_workspaces` / `monitored_models` are `None` when MPM presence was not - collected, so a run without --mpm never reads as zero adoption. - Monitored models come from the registry `is_monitored` flag, NOT - `nb_models_registered` (which counts every registry model).""" + collected, so a run without --mpm never reads as zero adoption. When it + was only partly collected they are sums over the known workspaces -- a + lower bound -- and `mpm_unchecked` (workspaces whose model count is + unknown) says how much is missing; `mpm_checked` counts workspaces whose + MPM flag is known. Monitored models come from the registry `is_monitored` + flag, NOT `nb_models_registered` (which counts every registry model).""" reported = mpm_reported(workspaces) return { "workspaces": len(workspaces), @@ -820,6 +826,8 @@ def workspace_org_totals(workspaces: "list[WorkspaceRecord]") -> dict: "monitored_models": ( sum(w.num_monitored_models or 0 for w in workspaces) if reported else None ), + "mpm_checked": sum(1 for w in workspaces if w.mpm_enabled is not None), + "mpm_unchecked": sum(1 for w in workspaces if w.num_monitored_models is None), } diff --git a/tests/unit/test_admin_growth_mpm.py b/tests/unit/test_admin_growth_mpm.py index 75f4603..ff621a1 100644 --- a/tests/unit/test_admin_growth_mpm.py +++ b/tests/unit/test_admin_growth_mpm.py @@ -137,6 +137,19 @@ def test_apply_mpm_presence_leaves_unknown_workspaces_untouched(): assert (ws["c"].mpm_enabled, ws["c"].num_monitored_models) == (None, None) +def test_apply_mpm_presence_drops_stale_fields_on_failed_lookup(): + from cometx.cli.admin_growth_mpm import apply_mpm_presence + from cometx.cli.admin_growth_users import parse_workspaces + + cb = { + "workspaces": [ + {"name": "w", "mpmEnabled": True, "monitoredModels": [{"id": "old"}]} + ] + } + (w,) = parse_workspaces(apply_mpm_presence(cb, {"w": None})) + assert (w.mpm_enabled, w.num_monitored_models) == (None, None) + + def _chargeback(): return { "workspaces": [ @@ -188,8 +201,29 @@ def test_build_with_mpm_merges_presence_and_reports_unchecked(): assert by_name["mpm_workspaces"] == 1 assert by_name["mpm_workspaces_unchecked"] == 1 overview = report["sections"]["unified"] - models_kpi = next(k for k in overview["kpis"] if k["label"] == "Monitored models") + kpis_by_label = {k["label"]: k for k in overview["kpis"]} + models_kpi = kpis_by_label["Monitored models"] + # partial: a lower bound, labeled as such + assert models_kpi["value"] == "≥ 1" assert models_kpi["sub"] == "MPM checked in 1/2 workspaces" + ws_kpi = kpis_by_label["MPM workspaces"] + # percentage over the checked workspace, not all workspaces + assert ws_kpi["sub"] == "100% of 1 checked; 1 unknown" + + +def test_build_with_mpm_all_lookups_failed_still_reports_unchecked(): + from cometx.cli.admin_growth_report import GrowthReporter + + api = _api( + member_error=RuntimeError("404"), + registry={"fraud": RuntimeError("403"), "credit": RuntimeError("403")}, + ) + r = GrowthReporter(api, window="7d", units="month", mpm=True) + r.build([], chargeback=_chargeback()) + _users, _ws, kpis = r.last_parsed() + by_name = {k[0]: k[1] for k in kpis} + assert "mpm_workspaces" not in by_name + assert by_name["mpm_workspaces_unchecked"] == 2 def test_build_with_mpm_only_checks_scoped_workspaces(): diff --git a/tests/unit/test_admin_growth_users.py b/tests/unit/test_admin_growth_users.py index 269f805..18a3708 100644 --- a/tests/unit/test_admin_growth_users.py +++ b/tests/unit/test_admin_growth_users.py @@ -3,6 +3,8 @@ """Unit tests for cometx.cli.admin_growth_users (people layer parse + adoption/leaderboard derivations).""" +import pytest + NOW = 1_720_000_000_000 # fixed ms; ~2024-07, tests pass now_ms explicitly @@ -611,6 +613,8 @@ def test_parse_workspaces_org_totals_and_platform_mix(): # no MPM fields in the payload -> not reported, not zero "mpm_workspaces": None, "monitored_models": None, + "mpm_checked": 0, + "mpm_unchecked": 4, } # em-ws=EM only; opik-ws=Opik only (b has spans); both-ws=both; empty-ws=neither assert platform_mix(ws, parse_users(cb)) == { @@ -810,3 +814,38 @@ def test_parse_workspaces_mpm_fields(): totals = workspace_org_totals(list(ws.values())) assert totals["mpm_workspaces"] == 4 # fraud, credit, risk, ops assert totals["monitored_models"] == 6 # 3 + 2 + 1 (+ ops unknown) + + +@pytest.mark.parametrize("raw", [1.5, -1, -2.0]) +def test_parse_workspaces_rejects_malformed_model_counts(raw): + from cometx.cli.admin_growth_users import parse_workspaces + + (w,) = parse_workspaces({"workspaces": [{"name": "w", "monitoredModels": raw}]}) + assert w.num_monitored_models is None + assert w.mpm_enabled is None + + +def test_parse_workspaces_accepts_whole_float_count(): + from cometx.cli.admin_growth_users import parse_workspaces + + (w,) = parse_workspaces({"workspaces": [{"name": "w", "monitoredModels": 3.0}]}) + assert (w.mpm_enabled, w.num_monitored_models) == (True, 3) + + +def test_org_totals_count_unknown_workspaces(): + from cometx.cli.admin_growth_users import parse_workspaces, workspace_org_totals + + ws = parse_workspaces( + { + "workspaces": [ + {"name": "a", "mpmEnabled": True, "monitoredModels": [{"id": 1}]}, + {"name": "b"}, # unknown + {"name": "c", "mpmEnabled": True}, # flag only: count unknown + ] + } + ) + totals = workspace_org_totals(ws) + assert totals["mpm_workspaces"] == 2 + assert totals["monitored_models"] == 1 # lower bound + assert totals["mpm_checked"] == 2 # a, c have a known flag + assert totals["mpm_unchecked"] == 2 # b, c have no known count From 0afbedadcf76345fc72dce31ed21537e7959def2 Mon Sep 17 00:00:00 2001 From: Douglas Blank Date: Wed, 30 Sep 2026 12:20:46 -0700 Subject: [PATCH 7/9] fix(admin): classify and surface mpm/v3/workspaces failures under --mpm Any failure of the member lookup used to be swallowed, with a silent fall back to the registry. That included 401/403 (e.g. under SageMaker auth) and proxy routing misses. A refused key that is also not an org admin then got a confident-looking low count instead of a visible problem, because the registry hides private models in non-member workspaces. The lookup result is now classified: ok, refused (401/403), not_found (404: MPM not installed or not routed), or error (anything else, including an unexpected response shape). Each failure prints a warning naming the URL and status; for refused, the warning also says the registry fallback may undercount. The result is recorded as an mpm_member_lookup label KPI in growth_org_kpis.csv, and in the HTML notes, including the Monitored models card even when every workspace was checked. http_error_status moves to cometx.utils so the MPM module can share it with the chargeback error messages without a circular import. The module docstring records why the /api/mpm/v3/workspaces path is fixed. Co-Authored-By: Claude Opus 5.5 (1M context) --- README-ADMIN.md | 2 +- cometx/cli/admin_growth_csv.py | 12 +++- cometx/cli/admin_growth_mpm.py | 92 +++++++++++++++++++++++------ cometx/cli/admin_growth_report.py | 50 +++++++++------- cometx/utils.py | 11 ++++ tests/unit/test_admin_growth_mpm.py | 92 +++++++++++++++++++++++++++-- 6 files changed, 214 insertions(+), 45 deletions(-) diff --git a/README-ADMIN.md b/README-ADMIN.md index 45c2382..c73a95f 100644 --- a/README-ADMIN.md +++ b/README-ADMIN.md @@ -257,7 +257,7 @@ Use the `deleted_users` KPI to see how many were excluded. **`growth_workspaces.csv`**: `report_date, workspace, member_count, num_projects, num_experiments, data_mb, mpm_enabled, num_monitored_models` -`mpm_enabled` is `1`/`0` (so `SUM(mpm_enabled)` counts MPM workspaces) and `num_monitored_models` is a count; both are empty unless `--mpm` was given and that workspace could be checked. With `--mpm`, `growth_org_kpis.csv` also gets `mpm_workspaces`, `total_monitored_models`, and `mpm_workspaces_unchecked`. The two totals cover only the workspaces that could be checked, so they are exact only when `mpm_workspaces_unchecked` is 0; otherwise they are lower bounds, and the HTML report labels them that way. +`mpm_enabled` is `1`/`0` (so `SUM(mpm_enabled)` counts MPM workspaces) and `num_monitored_models` is a count; both are empty unless `--mpm` was given and that workspace could be checked. With `--mpm`, `growth_org_kpis.csv` also gets `mpm_workspaces`, `total_monitored_models`, `mpm_workspaces_unchecked`, and `mpm_member_lookup`. That last one is a `label` metric: its `metric_text` says how the `/api/mpm/v3/workspaces` call went (`ok`, `refused` for a 401/403, `not_found` for a 404, or `error`). Anything but `ok` means every workspace was checked through the registry instead. `refused` is the one to watch: the counts may be low, because the registry hides private models in other workspaces from users who aren't organization admins. The two totals cover only the workspaces that could be checked, so they are exact only when `mpm_workspaces_unchecked` is 0; otherwise they are lower bounds, and the HTML report labels them that way. **`growth_org_kpis.csv`**: `report_date, metric_name, metric_value, metric_unit, metric_text` — long format so new metrics arrive as new rows without ever changing the Glue schema. `metric_unit` is one of `count`, `percent`, `megabytes`, `label`. diff --git a/cometx/cli/admin_growth_csv.py b/cometx/cli/admin_growth_csv.py index d05425b..9f4dd20 100644 --- a/cometx/cli/admin_growth_csv.py +++ b/cometx/cli/admin_growth_csv.py @@ -247,7 +247,7 @@ def collect_org_kpis( active_window_days, scope=None, excluded_personal_count=0, - mpm_requested=False, + mpm_member_lookup=None, ): """Flatten the report's org-level numbers into (name, value, unit) triples. @@ -356,12 +356,20 @@ def collect_org_kpis( # is never mistaken for a complete one -- the totals are exact only when # it is 0. Emitted whenever MPM was requested or reported, including a # run where every lookup failed. + # + # `mpm_member_lookup` (a label: ok / refused / not_found / error) records + # how the mpm/v3/workspaces call went, so a dashboard can tell "MPM not + # installed" from "the key was refused" -- in the latter case the registry + # fallback may have missed private models. `None` when --mpm was not given. org = workspace_org_totals(ws_records) + requested = mpm_member_lookup is not None if org["mpm_workspaces"] is not None: kpis.append(("mpm_workspaces", org["mpm_workspaces"], "count")) kpis.append(("total_monitored_models", org["monitored_models"], "count")) - if mpm_requested or org["mpm_workspaces"] is not None: + if requested or org["mpm_workspaces"] is not None: kpis.append(("mpm_workspaces_unchecked", org["mpm_unchecked"], "count")) + if requested: + kpis.append(("mpm_member_lookup", None, "label", mpm_member_lookup)) if split is not None: for bucket in ("personal", "service"): diff --git a/cometx/cli/admin_growth_mpm.py b/cometx/cli/admin_growth_mpm.py index f44448d..28322dc 100644 --- a/cometx/cli/admin_growth_mpm.py +++ b/cometx/cli/admin_growth_mpm.py @@ -23,7 +23,12 @@ Both sources below apply exactly that predicate server-side: 1. `GET /api/mpm/v3/workspaces` -- one call returning every workspace the - API key's user is a MEMBER of, with its monitored models. + API key's user is a MEMBER of, with its monitored models. The path is + fixed on purpose: it is a backend-react route (registered in + ReactWebappServerApplication), not an MPM-service one, so it resolves the + same way on Cloud and on chart deployments. If it fails, the failure is + classified (refused / not_found / error), warned about, and recorded, so + "MPM not installed" is distinguishable from "key refused". 2. For the remaining workspaces (an org admin need not be a member), the REST v2 registry: list the workspace's models, then read each model's details for its monitored flag. Org admins may read any workspace this way, @@ -45,11 +50,24 @@ import concurrent.futures -from cometx.utils import admin_api_url +from cometx.utils import ( + admin_api_url, + exception_text, + http_error_status, + redact_url_userinfo, +) MPM_WORKSPACES_PATH = "/api/mpm/v3/workspaces" DEFAULT_MAX_WORKERS = 8 +# Outcome of the `mpm/v3/workspaces` call, kept so the report and CSV can tell +# "MPM not installed" apart from "the key was refused". Every value but +# LOOKUP_OK means the registry covered every workspace instead. +LOOKUP_OK = "ok" +LOOKUP_REFUSED = "refused" # 401/403 +LOOKUP_NOT_FOUND = "not_found" # 404: MPM not installed, or not routed +LOOKUP_ERROR = "error" # anything else: 5xx, network, unexpected shape + def _mpm_url(api) -> str: """URL of `mpm/v3/workspaces`, beside (not under) the SDK's `/clientlib` @@ -58,11 +76,41 @@ def _mpm_url(api) -> str: return admin_api_url(api.config["comet.url_override"], MPM_WORKSPACES_PATH) -def _fetch_member_workspaces(api) -> "dict[str, list[dict]] | None": - """`mpm/v3/workspaces` -> {workspace_name: [{"id", "name"}, ...]} for the - workspaces the caller belongs to. `None` on any failure (MPM disabled on - the deployment, network error, unexpected shape): the registry path then - covers every workspace instead.""" +def _lookup_warning(lookup, exc, url) -> str: + """Operator-facing warning for a failed `mpm/v3/workspaces` call.""" + where = redact_url_userinfo(url) if url else MPM_WORKSPACES_PATH + detail = exception_text(exc) + if lookup == LOOKUP_REFUSED: + return ( + "Warning: the MPM API refused this key at %s (%s). Checking every " + "workspace via the model registry instead. There, workspaces the " + "key's user is not a member of show only public models unless " + "the user is an organization admin, so counts may be low." % (where, detail) + ) + if lookup == LOOKUP_NOT_FOUND: + return ( + "Warning: the MPM API was not found at %s (%s); MPM may not be " + "installed or routed on this deployment. Checking every workspace " + "via the model registry instead." % (where, detail) + ) + return ( + "Warning: the MPM API call to %s failed (%s). Checking every " + "workspace via the model registry instead." % (where, detail) + ) + + +def _fetch_member_workspaces(api) -> "tuple[dict[str, list[dict]] | None, str]": + """`mpm/v3/workspaces` -> ({workspace_name: [{"id", "name"}, ...]}, lookup) + for the workspaces the caller belongs to. + + On failure the dict is `None` (the registry path then covers every + workspace) and `lookup` says why -- LOOKUP_REFUSED, LOOKUP_NOT_FOUND or + LOOKUP_ERROR -- with a warning printed. Failures are classified rather + than swallowed because the fallback is only as complete as the key's + registry access: a refused key that also isn't an org admin would + otherwise yield a confident-looking low count instead of a visible + problem.""" + url = None try: url = _mpm_url(api) response = api._client.get( @@ -79,9 +127,17 @@ def _fetch_member_workspaces(api) -> "dict[str, list[dict]] | None": for m in (ws.get("models") or []) if isinstance(m, dict) ] - return out - except Exception: - return None + except Exception as exc: + status = http_error_status(exc) + if status in (401, 403): + lookup = LOOKUP_REFUSED + elif status == 404: + lookup = LOOKUP_NOT_FOUND + else: + lookup = LOOKUP_ERROR + print(_lookup_warning(lookup, exc, url)) + return None, lookup + return out, LOOKUP_OK def _is_monitored(details) -> "bool | None": @@ -158,13 +214,15 @@ def check(pair): def fetch_mpm_presence( api, workspace_names, max_workers=DEFAULT_MAX_WORKERS -) -> "dict[str, list[dict] | None]": - """Monitored models per workspace: {name: [{"id", "name"}, ...]}, or - {name: None} where it could not be determined. Workspaces in - `mpm/v3/workspaces` are answered from that single call; the rest fall - back to the registry.""" +) -> "tuple[dict[str, list[dict] | None], str]": + """(presence, lookup). `presence` maps each workspace to its monitored + models, [{"id", "name"}, ...], or to `None` where they could not be + determined. Workspaces in `mpm/v3/workspaces` are answered from that + single call; the rest fall back to the registry. `lookup` is how that + call went (LOOKUP_OK or a failure class; see `_fetch_member_workspaces`).""" names = list(dict.fromkeys(n for n in workspace_names if n)) - member = _fetch_member_workspaces(api) or {} + member, lookup = _fetch_member_workspaces(api) + member = member or {} presence = {n: member[n] for n in names if n in member} remaining = [n for n in names if n not in presence] if remaining: @@ -175,7 +233,7 @@ def fetch_mpm_presence( presence.update( _registry_monitored_models(api, remaining, max_workers=max_workers) ) - return presence + return presence, lookup def apply_mpm_presence(chargeback, presence) -> dict: diff --git a/cometx/cli/admin_growth_report.py b/cometx/cli/admin_growth_report.py index 83608f1..6a7ea4e 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -58,6 +58,7 @@ exception_text, fetch_chargeback_report, format_time_key, + http_error_status, redact_url_userinfo, ) @@ -211,21 +212,11 @@ def _fetch_service_accounts(api) -> "set[str] | None": return None -def _error_status(exc) -> "int | None": - """HTTP status of a failed request: from the SDK exception's `response` - when it has one, else parsed from its text (`status_code: NNN`).""" - status = getattr(getattr(exc, "response", None), "status_code", None) - if isinstance(status, int): - return status - match = re.search(r"status_code:\s*(\d+)", exception_text(exc)) - return int(match.group(1)) if match else None - - def _chargeback_error_message(exc) -> str: """Say what actually went wrong fetching the chargeback report, rather than attributing every failure to a non-admin key: a 404 (wrong URL or path prefix) or a 5xx says nothing about the key.""" - status = _error_status(exc) + status = http_error_status(exc) response = getattr(exc, "response", None) url = getattr(response, "url", None) # Prefer "HTTP 401: " over the SDK exception's text, which @@ -463,6 +454,9 @@ def __init__( self.api = api # --mpm: collect MPM presence client-side (see admin_growth_mpm). self.mpm = mpm + # How the mpm/v3/workspaces call went on the last --mpm build (see + # admin_growth_mpm.LOOKUP_*); None when --mpm was not given. + self._mpm_member_lookup = None self.window = window self.units = units self.active_window = active_window @@ -538,6 +532,7 @@ def build(self, workspaces, chargeback=None): chargeback = self._filter_personal_chargeback(chargeback) excluded_personal_count = before - len((chargeback.get("workspaces") or [])) scope = set(workspaces) if workspaces else None + self._mpm_member_lookup = None if self.mpm: chargeback = self._add_mpm_presence(chargeback, scope) print("Building report...") @@ -572,7 +567,7 @@ def _add_mpm_presence(self, chargeback, scope): if w.get("name") and (scope is None or w.get("name") in scope) ] print("Collecting MPM presence for %d workspace(s)..." % len(names)) - presence = fetch_mpm_presence(self.api, names) + presence, self._mpm_member_lookup = fetch_mpm_presence(self.api, names) checked = sum(1 for n in names if presence.get(n) is not None) if checked < len(names): print( @@ -583,20 +578,30 @@ def _add_mpm_presence(self, chargeback, scope): return apply_mpm_presence(chargeback, presence) def _mpm_note(self, ws_records): - """Short provenance text for MPM figures in hints/KPI subs.""" + """Short provenance text for MPM figures in hints/KPI subs. Says when + the mpm/v3/workspaces call failed, so "MPM not installed" and "key + refused" never look like a clean run.""" + lookup_notes = { + "refused": "MPM API refused this key; counts may be low", + "not_found": "MPM API not found on this server", + "error": "MPM API call failed", + } + lookup_note = lookup_notes.get(self._mpm_member_lookup) if not mpm_reported(ws_records): - return ( - "MPM not collected (run with --mpm)" - if not self.mpm - else "MPM could not be determined" + if not self.mpm: + return "MPM not collected (run with --mpm)" + return "MPM could not be determined" + ( + "; " + lookup_note if lookup_note else "" ) org = workspace_org_totals(ws_records) if org["mpm_unchecked"]: - return "MPM checked in %d/%d workspaces" % ( + note = "MPM checked in %d/%d workspaces" % ( org["workspaces"] - org["mpm_unchecked"], org["workspaces"], ) - return "MPM shown separately" + else: + note = "MPM shown separately" + return note + ("; " + lookup_note if lookup_note else "") def _empty_export_reason(self, scope, excluded_personal_count): """A block reason when the filters left nothing to export, else None. @@ -961,6 +966,7 @@ def _build_unified_section( } ) partial = org["mpm_unchecked"] > 0 + lookup_failed = self._mpm_member_lookup not in (None, "ok") kpis.append( { "label": "Monitored models", @@ -970,7 +976,9 @@ def _build_unified_section( else org["monitored_models"] ), "sub": ( - self._mpm_note(ws_records) if partial else "MPM (is_monitored)" + self._mpm_note(ws_records) + if partial or lookup_failed + else "MPM (is_monitored)" ), } ) @@ -1604,7 +1612,7 @@ def _assemble_report_data( active_window_days, scope=scope, excluded_personal_count=excluded_personal_count, - mpm_requested=self.mpm, + mpm_member_lookup=self._mpm_member_lookup, ), ) except Exception as exc: diff --git a/cometx/utils.py b/cometx/utils.py index a638a57..fdd9e5c 100644 --- a/cometx/utils.py +++ b/cometx/utils.py @@ -430,6 +430,17 @@ def exception_text(exc): return type(exc).__name__ +def http_error_status(exc): + """HTTP status of a failed request, or `None`: from the SDK exception's + `response` when it has one, else parsed from its text + (`status_code: NNN`).""" + status = getattr(getattr(exc, "response", None), "status_code", None) + if isinstance(status, int) and not isinstance(status, bool): + return status + match = re.search(r"status_code:\s*(\d+)", exception_text(exc)) + return int(match.group(1)) if match else None + + def redact_url_userinfo(value): """Replace any `user:password@` userinfo in `value` with `***@`. diff --git a/tests/unit/test_admin_growth_mpm.py b/tests/unit/test_admin_growth_mpm.py index ff621a1..c793abb 100644 --- a/tests/unit/test_admin_growth_mpm.py +++ b/tests/unit/test_admin_growth_mpm.py @@ -57,8 +57,9 @@ def test_member_workspaces_answered_from_one_call(): ] } ) - presence = fetch_mpm_presence(api, ["fraud", "research"]) + presence, lookup = fetch_mpm_presence(api, ["fraud", "research"]) assert presence == {"fraud": [{"id": "m1", "name": "scorer"}], "research": []} + assert lookup == "ok" # every requested workspace was covered by v3: no registry calls api._client.get_registry_models.assert_not_called() @@ -76,7 +77,7 @@ def test_non_member_workspaces_fall_back_to_registry(): ("credit", "ead"): {"isMonitored": False}, }, ) - presence = fetch_mpm_presence(api, ["fraud", "credit"]) + presence, _lookup = fetch_mpm_presence(api, ["fraud", "credit"]) assert presence["fraud"] == [] # sorted by name for stable output assert presence["credit"] == [ @@ -93,8 +94,9 @@ def test_v3_failure_uses_registry_for_everything(): registry={"a": ["m"], "b": []}, details={("a", "m"): {"isMonitored": True}}, ) - presence = fetch_mpm_presence(api, ["a", "b"]) + presence, lookup = fetch_mpm_presence(api, ["a", "b"]) assert presence == {"a": [{"id": "id-m", "name": "m"}], "b": []} + assert lookup == "error" # a bare exception with no HTTP status @pytest.mark.parametrize( @@ -115,7 +117,7 @@ def test_unknown_is_none_not_empty(registry, details): from cometx.cli.admin_growth_mpm import fetch_mpm_presence api = _api(registry=registry, details=details) - assert fetch_mpm_presence(api, ["w"]) == {"w": None} + assert fetch_mpm_presence(api, ["w"])[0] == {"w": None} def test_apply_mpm_presence_leaves_unknown_workspaces_untouched(): @@ -259,3 +261,85 @@ def test_mpm_url_drops_sdk_clientlib_segment(override, expected): api = MagicMock() api.config = {"comet.url_override": override} assert _mpm_url(api) == expected + + +def _status_error(status): + exc = RuntimeError("GET failed with status code %d" % status) + exc.response = MagicMock(status_code=status) + return exc + + +@pytest.mark.parametrize( + "error, expected, warning", + [ + (_status_error(401), "refused", "refused this key"), + (_status_error(403), "refused", "refused this key"), + (_status_error(404), "not_found", "may not be installed"), + (_status_error(502), "error", "failed"), + (RuntimeError("connection reset"), "error", "failed"), + ], +) +def test_member_lookup_failures_are_classified_and_warned( + capsys, error, expected, warning +): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api(member_error=error, registry={"w": []}) + presence, lookup = fetch_mpm_presence(api, ["w"]) + assert lookup == expected + # the fallback still runs + assert presence == {"w": []} + out = capsys.readouterr().out + assert "Warning: the MPM API" in out and warning in out + if expected == "refused": + # the refused case says why the fallback may undercount + assert "organization admin" in out + + +def test_member_lookup_shape_error_is_error_not_ok(): + from cometx.cli.admin_growth_mpm import fetch_mpm_presence + + api = _api(member_payload={"unexpected": []}, registry={"w": []}) + _presence, lookup = fetch_mpm_presence(api, ["w"]) + assert lookup == "error" + + +@pytest.mark.parametrize( + "error, lookup, note", + [ + (None, "ok", None), + (_status_error(401), "refused", "MPM API refused this key; counts may be low"), + (_status_error(404), "not_found", "MPM API not found on this server"), + ], +) +def test_build_records_member_lookup_in_csv_and_html(error, lookup, note): + from cometx.cli.admin_growth_report import GrowthReporter + + api = _api( + member_error=error, + member_payload={"workspaces": [{"workspaceName": "fraud", "models": []}]}, + registry={"fraud": [], "credit": []}, + ) + r = GrowthReporter(api, window="7d", units="month", mpm=True) + report = r.build([], chargeback=_chargeback()) + _users, _ws, kpis = r.last_parsed() + text_by_name = {k[0]: k[3] for k in kpis} + assert text_by_name["mpm_member_lookup"] == lookup + models_kpi = next( + k + for k in report["sections"]["unified"]["kpis"] + if k["label"] == "Monitored models" + ) + if note: + # even with every workspace checked, a failed lookup is flagged + assert note in models_kpi["sub"] + else: + assert models_kpi["sub"] == "MPM (is_monitored)" + + +def test_build_without_mpm_has_no_member_lookup_kpi(): + from cometx.cli.admin_growth_report import GrowthReporter + + r = GrowthReporter(_api(), window="7d", units="month") + r.build([], chargeback=_chargeback()) + assert "mpm_member_lookup" not in {k[0] for k in r.last_parsed()[2]} From ab133f15adfc9f2c2b679f28da2cf0187ce4de8f Mon Sep 17 00:00:00 2001 From: Nimrod Lahav Date: Thu, 1 Oct 2026 10:36:50 +0300 Subject: [PATCH 8/9] fix(admin): take an HTTP status only from a real response Baz's finding on the classification added in 0afbeda, and it is right. http_error_status fell back to parsing `status_code: NNN` out of the exception's text when there was no response. Callers route on the result -- 401/403 means the key was refused, 404 means MPM is not installed or not routed -- so a status recovered from incidental text does not degrade to "unknown". It asserts something specific and wrong, which is then recorded in the mpm_member_lookup KPI and rendered in the HTML. A connection reset quoting an inner frame became "MPM is not installed here". That is the failure the classification was added to prevent, so the fallback defeated the commit it shipped in. Nothing is lost by refusing to guess, because the branch was unreachable for the errors it was meant to serve. Every HTTP failure the SDK raises carries a response: CometRestApiException.__init__ always assigns one, and NotFound and Unauthorized both subclass it, so the first branch returns. Its own __str__ renders "failed with status code 404" -- no colon -- so the pattern would not have matched that text even if it were reached. The branch could only ever fire for a non-HTTP exception, where any match is incidental by definition. One failure mode, no upside. _short_api_error in admin_growth_report matches the same shape and keeps it deliberately: there the number is only displayed, so a wrong match is cosmetic rather than a misrouted classification. The docstring says so, since the two now differ on purpose. Four tests, including the connection-error case that regressed it. Verified as guards: restoring the fallback fails the text case. pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) --- cometx/cli/admin_gpu_app.py | 17 ++++++-- cometx/cli/admin_gpu_report.py | 4 +- cometx/cli/copy.py | 15 +++---- cometx/cli/rename_duplicates.py | 8 +--- cometx/framework/comet/download_manager.py | 4 +- cometx/utils.py | 26 ++++++++--- examples/00_seed_demo_project.py | 32 +++++++++++--- examples/01_inspect_views.py | 15 +++++-- examples/02_build_project_dashboard.py | 32 +++++++++++--- examples/03_build_experiment_view.py | 23 +++++++--- examples/04_migrate_views.py | 50 +++++++++++++++------ examples/05_standardize_workspace.py | 40 +++++++++++++---- examples/06_harvest_panels.py | 51 ++++++++++++++-------- tests/unit/test_utils.py | 49 +++++++++++++++++++++ 14 files changed, 275 insertions(+), 91 deletions(-) diff --git a/cometx/cli/admin_gpu_app.py b/cometx/cli/admin_gpu_app.py index 2eb6d67..8d2a704 100644 --- a/cometx/cli/admin_gpu_app.py +++ b/cometx/cli/admin_gpu_app.py @@ -478,7 +478,10 @@ def get_available_workspaces_and_projects(data: Dict) -> Dict[str, List[str]]: ws = exp_info.get("workspace", "Unknown") proj = exp_info.get("project_name", "Unknown") workspace_projects[ws].add(proj) - return {ws: sorted(list(projects)) for ws, projects in sorted(workspace_projects.items())} + return { + ws: sorted(list(projects)) + for ws, projects in sorted(workspace_projects.items()) + } def get_available_dates(data: Dict) -> Tuple[List[str], str, str]: @@ -532,6 +535,7 @@ def main(): # This must be done before any Streamlit elements are rendered try: import streamlit.config as _config + _config.set_option("theme.primaryColor", "#1f77b4") except Exception: pass # Fallback gracefully if internal API changes @@ -602,7 +606,8 @@ def main(): all_workspaces = list(workspace_projects_map.keys()) # Custom CSS for professional styling (override Streamlit's default red/pink accent) - st.markdown(""" + st.markdown( + """ - """, unsafe_allow_html=True) + """, + unsafe_allow_html=True, + ) # Sidebar configuration with st.sidebar: @@ -666,7 +673,9 @@ def main(): st.rerun() # Expandable project selector for fine-grained control - with st.expander("Select specific projects", expanded=st.session_state.projects_expander_open): + with st.expander( + "Select specific projects", expanded=st.session_state.projects_expander_open + ): # Group projects by workspace for cleaner display for ws in all_workspaces: ws_projects = [f"{ws}/{p}" for p in workspace_projects_map.get(ws, [])] diff --git a/cometx/cli/admin_gpu_report.py b/cometx/cli/admin_gpu_report.py index e8cbb82..dcdeb18 100644 --- a/cometx/cli/admin_gpu_report.py +++ b/cometx/cli/admin_gpu_report.py @@ -21,8 +21,6 @@ import matplotlib.pyplot as plt from comet_ml import API - -from .admin_utils import get_distinct_colors, save_chart from comet_ml.query import Metadata from PIL import Image from reportlab.lib import colors @@ -44,6 +42,8 @@ ) from rich.table import Table +from .admin_utils import get_distinct_colors, save_chart + # Suppress matplotlib warnings about non-GUI backend warnings.filterwarnings("ignore", category=UserWarning, module="matplotlib") diff --git a/cometx/cli/copy.py b/cometx/cli/copy.py index 11357ad..53929f7 100644 --- a/cometx/cli/copy.py +++ b/cometx/cli/copy.py @@ -78,10 +78,9 @@ import time import urllib.parse import zipfile - -import requests from datetime import datetime, timedelta +import requests from comet_ml import APIExperiment, Artifact, Experiment, OfflineExperiment from comet_ml._typing import TemporaryFilePath from comet_ml.file_uploader import GitPatchUploadProcessor @@ -800,7 +799,9 @@ def cleanup(self): # Restore default signal handler signal.signal(signal.SIGINT, signal.default_int_handler) - def copy(self, source, destination, symlink, ignore, debug, sync, create_workspaces=False): + def copy( + self, source, destination, symlink, ignore, debug, sync, create_workspaces=False + ): """ """ self.ignore = ignore self.debug = debug @@ -1231,9 +1232,7 @@ def update_datagrid_contents( else: asset_map[old_asset_id] = result["assetId"] - def _log_asset( - self, experiment, path, asset_type, asset_data, asset_map - ): + def _log_asset(self, experiment, path, asset_type, asset_data, asset_map): log_as_filename = asset_data.get("logAsFileName", None) original_filename = asset_data["fileName"] disk_filename = asset_data.get("diskFileName", original_filename) @@ -1397,7 +1396,7 @@ def _log_asset( # The dir field includes a "models/" prefix added by the # backend; strip it to get the actual model name. if dir_name.startswith("models/"): - model_name = dir_name[len("models/"):] + model_name = dir_name[len("models/") :] else: model_name = dir_name binary_io = open(filename, "rb") @@ -1443,7 +1442,7 @@ def log_assets(self, experiment, path, assets_metadata): if asset_type == "model-element": dir_name = asset_data.get("dir", "") if dir_name.startswith("models/"): - model_name = dir_name[len("models/"):] + model_name = dir_name[len("models/") :] else: model_name = dir_name raw_metadata = asset_data.get("metadata") diff --git a/cometx/cli/rename_duplicates.py b/cometx/cli/rename_duplicates.py index a154f69..4b09391 100644 --- a/cometx/cli/rename_duplicates.py +++ b/cometx/cli/rename_duplicates.py @@ -165,9 +165,7 @@ def rename_duplicates(parsed_args): # Get workspaces to process all_workspaces = api.get_workspaces() if filter_workspace: - workspaces_to_process = [ - w for w in all_workspaces if w == filter_workspace - ] + workspaces_to_process = [w for w in all_workspaces if w == filter_workspace] else: workspaces_to_process = all_workspaces @@ -191,9 +189,7 @@ def rename_duplicates(parsed_args): # Get projects for this workspace all_projects = api.get_projects(workspace) if filter_project: - projects_to_process = [ - p for p in all_projects if p == filter_project - ] + projects_to_process = [p for p in all_projects if p == filter_project] else: projects_to_process = all_projects diff --git a/cometx/framework/comet/download_manager.py b/cometx/framework/comet/download_manager.py index 20b9294..ce3feb2 100644 --- a/cometx/framework/comet/download_manager.py +++ b/cometx/framework/comet/download_manager.py @@ -1264,9 +1264,7 @@ def download_assets(self, experiment): path = assets_path else: path = os.path.join(assets_path, asset_type) - filename = sanitize_filename( - asset.get("diskFileName", asset["fileName"]) - ) + filename = sanitize_filename(asset.get("diskFileName", asset["fileName"])) file_path = os.path.join(path, filename) if file_path not in filenames and self._should_write(file_path): filenames.add(file_path) diff --git a/cometx/utils.py b/cometx/utils.py index fdd9e5c..ba881b5 100644 --- a/cometx/utils.py +++ b/cometx/utils.py @@ -431,14 +431,30 @@ def exception_text(exc): def http_error_status(exc): - """HTTP status of a failed request, or `None`: from the SDK exception's - `response` when it has one, else parsed from its text - (`status_code: NNN`).""" + """HTTP status of a failed request, or `None`. + + Taken only from a real response, never parsed out of the exception's text. + Callers route on this -- 401/403 means the key was refused, 404 means MPM + is not installed or not routed -- so a status guessed from incidental text + does not degrade to "unknown", it asserts something specific and wrong, + and that answer is then recorded and rendered. + + There is nothing to lose by refusing to guess: every HTTP failure the SDK + raises carries a response. `CometRestApiException.__init__` always assigns + one, and `NotFound` and `Unauthorized` both subclass it. So the text branch + could only ever fire for a *non*-HTTP exception -- a connection reset, a + DNS failure, a parse error -- where any `status_code: NNN` in the message + is by definition incidental to some inner frame. It had no upside and one + failure mode. + + `_short_api_error` in admin_growth_report matches the same shape on + purpose and should keep doing so: there the number is only displayed, so a + wrong match is cosmetic rather than a misrouted classification. + """ status = getattr(getattr(exc, "response", None), "status_code", None) if isinstance(status, int) and not isinstance(status, bool): return status - match = re.search(r"status_code:\s*(\d+)", exception_text(exc)) - return int(match.group(1)) if match else None + return None def redact_url_userinfo(value): diff --git a/examples/00_seed_demo_project.py b/examples/00_seed_demo_project.py index 3cb9743..0682715 100644 --- a/examples/00_seed_demo_project.py +++ b/examples/00_seed_demo_project.py @@ -57,9 +57,13 @@ def log_one(workspace, project, index): noise = random.uniform(-0.02, 0.02) loss = max(0.01, 2.5 * (1 - progress) + noise) experiment.log_metric("loss", loss, step=step) - experiment.log_metric("accuracy", min(0.999, 0.1 + 0.88 * progress + noise), step=step) + experiment.log_metric( + "accuracy", min(0.999, 0.1 + 0.88 * progress + noise), step=step + ) experiment.log_metric("val_loss", loss * random.uniform(1.0, 1.3), step=step) - experiment.log_metric("val_accuracy", min(0.999, 0.1 + 0.84 * progress + noise), step=step) + experiment.log_metric( + "val_accuracy", min(0.999, 0.1 + 0.84 * progress + noise), step=step + ) try: import numpy @@ -73,9 +77,21 @@ def log_one(workspace, project, index): "per_class_metrics.csv", tabular_data=[ ["class", "precision", "recall"], - ["cat", round(random.uniform(0.7, 0.99), 3), round(random.uniform(0.7, 0.99), 3)], - ["dog", round(random.uniform(0.7, 0.99), 3), round(random.uniform(0.7, 0.99), 3)], - ["bird", round(random.uniform(0.7, 0.99), 3), round(random.uniform(0.7, 0.99), 3)], + [ + "cat", + round(random.uniform(0.7, 0.99), 3), + round(random.uniform(0.7, 0.99), 3), + ], + [ + "dog", + round(random.uniform(0.7, 0.99), 3), + round(random.uniform(0.7, 0.99), 3), + ], + [ + "bird", + round(random.uniform(0.7, 0.99), 3), + round(random.uniform(0.7, 0.99), 3), + ], ], headers=False, ) @@ -97,8 +113,10 @@ def main(): for index in range(args.count): key = log_one(workspace, args.project, index) print(" logged %s" % key) - print("\nDone. Now run: python 02_build_project_dashboard.py --workspace %s --project %s" - % (workspace, args.project)) + print( + "\nDone. Now run: python 02_build_project_dashboard.py --workspace %s --project %s" + % (workspace, args.project) + ) if __name__ == "__main__": diff --git a/examples/01_inspect_views.py b/examples/01_inspect_views.py index 9f80a7e..a547e31 100644 --- a/examples/01_inspect_views.py +++ b/examples/01_inspect_views.py @@ -63,7 +63,9 @@ def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--workspace") parser.add_argument("--project", default="view-api-demo") - parser.add_argument("--json", action="store_true", help="dump every field of every view") + parser.add_argument( + "--json", action="store_true", help="dump every field of every view" + ) args = parser.parse_args() comet_ml.login() @@ -83,10 +85,15 @@ def main(): # The same call, widened to the whole workspace. Useful for finding a # dashboard you built in another project and want to reuse here. - workspace_views = api.get_views(workspace, args.project, include_workspace_views=True) + workspace_views = api.get_views( + workspace, args.project, include_workspace_views=True + ) extra = len(workspace_views) - len(project_views) - print("include_workspace_views=True adds %d view(s) from sibling projects" % max(0, extra)) - for view in workspace_views[len(project_views):][:10]: + print( + "include_workspace_views=True adds %d view(s) from sibling projects" + % max(0, extra) + ) + for view in workspace_views[len(project_views) :][:10]: print(" - %r (project_id=%s)" % (view.name, view.project_id)) if extra > 10: print(" ... and %d more" % (extra - 10)) diff --git a/examples/02_build_project_dashboard.py b/examples/02_build_project_dashboard.py index c34c8d8..1ad5fd6 100644 --- a/examples/02_build_project_dashboard.py +++ b/examples/02_build_project_dashboard.py @@ -48,9 +48,15 @@ def build(name): columns=3, height=1, panels=[ - scalar_panel("accuracy", aggregation="max", description="Best accuracy reached"), + scalar_panel( + "accuracy", aggregation="max", description="Best accuracy reached" + ), scalar_panel("loss", aggregation="min", description="Lowest training loss"), - scalar_panel("val_accuracy", aggregation="last", description="Final validation accuracy"), + scalar_panel( + "val_accuracy", + aggregation="last", + description="Final validation accuracy", + ), ], ) @@ -60,8 +66,14 @@ def build(name): columns=2, panels=[ # Two metrics overlaid in a single chart. - line_panel(["loss", "val_loss"], name="Loss: train vs validation", smoothing=0.6), - line_panel(["accuracy", "val_accuracy"], name="Accuracy: train vs validation", smoothing=0.6), + line_panel( + ["loss", "val_loss"], name="Loss: train vs validation", smoothing=0.6 + ), + line_panel( + ["accuracy", "val_accuracy"], + name="Accuracy: train vs validation", + smoothing=0.6, + ), # Log-scale y-axis, and a legend keyed on a hyperparameter. line_panel( "loss", @@ -71,7 +83,9 @@ def build(name): ), # x-axis other than step; locked so the dashboard-wide x-axis # setting below does not override it. - line_panel("accuracy", x="duration", name="Accuracy over wall-clock", locked=True), + line_panel( + "accuracy", x="duration", name="Accuracy over wall-clock", locked=True + ), ], ) @@ -155,7 +169,9 @@ def main(): parser.add_argument("--workspace") parser.add_argument("--project", default="view-api-demo") parser.add_argument("--name", default="SDK Full Dashboard") - parser.add_argument("--dry-run", action="store_true", help="print the view instead of creating it") + parser.add_argument( + "--dry-run", action="store_true", help="print the view instead of creating it" + ) args = parser.parse_args() view = build(args.name) @@ -174,7 +190,9 @@ def main(): created = api.create_view(workspace, args.project, view) if not created: - raise SystemExit("create_view returned nothing -- the backend rejected the view.") + raise SystemExit( + "create_view returned nothing -- the backend rejected the view." + ) print("\nCreated %r (template_id=%s)" % (created.name, created.template_id)) print("Open: %s" % get_project_url(workspace, args.project)) diff --git a/examples/03_build_experiment_view.py b/examples/03_build_experiment_view.py index 13819da..63d5855 100644 --- a/examples/03_build_experiment_view.py +++ b/examples/03_build_experiment_view.py @@ -40,8 +40,14 @@ def build(name): "Summary", columns=3, panels=[ - scalar_panel("accuracy", aggregation="last", description="Accuracy at the last step"), - scalar_panel("val_accuracy", aggregation="max", description="Best validation accuracy"), + scalar_panel( + "accuracy", aggregation="last", description="Accuracy at the last step" + ), + scalar_panel( + "val_accuracy", + aggregation="max", + description="Best validation accuracy", + ), scalar_panel("loss", aggregation="min", description="Lowest loss"), ], ) @@ -83,8 +89,12 @@ def main(): parser.add_argument("--workspace") parser.add_argument("--project", default="view-api-demo") parser.add_argument("--name", default="Single-run Deep Dive") - parser.add_argument("--all", action="store_true", help="apply to every experiment in the project") - parser.add_argument("--dry-run", action="store_true", help="print the view instead of creating it") + parser.add_argument( + "--all", action="store_true", help="apply to every experiment in the project" + ) + parser.add_argument( + "--dry-run", action="store_true", help="print the view instead of creating it" + ) args = parser.parse_args() if args.dry_run: @@ -115,7 +125,10 @@ def main(): for experiment in targets: # Build a fresh view per experiment so panel ids stay unique. created = experiment.create_view(build(args.name)) - print(" [%s] %s %s" % ("OK" if created else "FAILED", experiment.id, experiment.name or "")) + print( + " [%s] %s %s" + % ("OK" if created else "FAILED", experiment.id, experiment.name or "") + ) print("\nReading back with Experiment.get_views():") for view in targets[0].get_views(): diff --git a/examples/04_migrate_views.py b/examples/04_migrate_views.py index b2e7b38..592acac 100644 --- a/examples/04_migrate_views.py +++ b/examples/04_migrate_views.py @@ -44,10 +44,16 @@ def parse_args(): parser.add_argument("--src-api-key", help="defaults to the configured key") parser.add_argument("--dst-api-key", help="defaults to the source key") parser.add_argument("--match", help="only copy views whose name contains this") - parser.add_argument("--include-transient", action="store_true", - help="also copy 'Unsaved Changes' views") - parser.add_argument("--experiment-views", action="store_true", - help="also copy the first experiment's own views") + parser.add_argument( + "--include-transient", + action="store_true", + help="also copy 'Unsaved Changes' views", + ) + parser.add_argument( + "--experiment-views", + action="store_true", + help="also copy the first experiment's own views", + ) parser.add_argument("--apply", action="store_true", help="perform the copy") return parser.parse_args() @@ -64,12 +70,19 @@ def main(): args = parse_args() comet_ml.login() - source = comet_ml.API(api_key=args.src_api_key) if args.src_api_key else comet_ml.API() + source = ( + comet_ml.API(api_key=args.src_api_key) if args.src_api_key else comet_ml.API() + ) destination = comet_ml.API(api_key=args.dst_api_key) if args.dst_api_key else source - print("=== Project-level views in %s/%s ===" % (args.src_workspace, args.src_project)) - project_views = [v for v in source.get_views(args.src_workspace, args.src_project) - if wanted(v, args)] + print( + "=== Project-level views in %s/%s ===" % (args.src_workspace, args.src_project) + ) + project_views = [ + v + for v in source.get_views(args.src_workspace, args.src_project) + if wanted(v, args) + ] for view in project_views: print(describe_view(view, indent=" ")) if not project_views: @@ -80,17 +93,28 @@ def main(): experiments = source.get_experiments(args.src_workspace, args.src_project) if experiments: print("\n=== Experiment-level views on %s ===" % experiments[0].id) - experiment_views = [v for v in experiments[0].get_views() if wanted(v, args)] + experiment_views = [ + v for v in experiments[0].get_views() if wanted(v, args) + ] for view in experiment_views: print(describe_view(view, indent=" ")) if not experiment_views: print(" (none matched)") if not args.apply: - print("\nDry run. Re-run with --apply to copy %d project view(s)%s to %s/%s." - % (len(project_views), - " and %d experiment view(s)" % len(experiment_views) if experiment_views else "", - args.dst_workspace, args.dst_project)) + print( + "\nDry run. Re-run with --apply to copy %d project view(s)%s to %s/%s." + % ( + len(project_views), + ( + " and %d experiment view(s)" % len(experiment_views) + if experiment_views + else "" + ), + args.dst_workspace, + args.dst_project, + ) + ) return print("\n=== Creating in %s/%s ===" % (args.dst_workspace, args.dst_project)) diff --git a/examples/05_standardize_workspace.py b/examples/05_standardize_workspace.py index dd2adb4..049c815 100644 --- a/examples/05_standardize_workspace.py +++ b/examples/05_standardize_workspace.py @@ -44,7 +44,11 @@ ("auc", True), ] -SYSTEM_METRICS = ["sys.cpu.percent.avg", "sys.ram.percent.used", "sys.gpu.0.gpu_utilization"] +SYSTEM_METRICS = [ + "sys.cpu.percent.avg", + "sys.ram.percent.used", + "sys.gpu.0.gpu_utilization", +] def project_metric_names(api, workspace, project): @@ -59,7 +63,11 @@ def project_metric_names(api, workspace, project): def build(available): """Build the standard view from whichever standard metrics are present.""" - present = [(name, higher_better) for name, higher_better in STANDARD_METRICS if name in available] + present = [ + (name, higher_better) + for name, higher_better in STANDARD_METRICS + if name in available + ] if not present: return None @@ -67,7 +75,10 @@ def build(available): scalar_panel(name, aggregation="max" if higher_better else "min") for name, higher_better in present[:6] ] - curves = [line_panel(name, name="%s over steps" % name, smoothing=0.4) for name, _ in present] + curves = [ + line_panel(name, name="%s over steps" % name, smoothing=0.4) + for name, _ in present + ] sections = [ Section("Key results", tiles, columns=3), @@ -76,8 +87,14 @@ def build(available): system = [m for m in SYSTEM_METRICS if m in available] if system: - sections.append(Section("System", [line_panel(m, name=m) for m in system], - columns=3, expanded=False)) + sections.append( + Section( + "System", + [line_panel(m, name=m) for m in system], + columns=3, + expanded=False, + ) + ) return build_view( VIEW_NAME, @@ -96,8 +113,11 @@ def main(): parser.add_argument("--workspace") parser.add_argument("--projects", nargs="*", help="limit to these projects") parser.add_argument("--limit", type=int, help="stop after N projects") - parser.add_argument("--update", action="store_true", - help="rebuild the view even if it already exists") + parser.add_argument( + "--update", + action="store_true", + help="rebuild the view even if it already exists", + ) parser.add_argument("--apply", action="store_true") args = parser.parse_args() @@ -145,8 +165,10 @@ def main(): print(" %-40s [%s] %d panels" % (project, "OK" if ok else "FAILED", panels)) created += ok - print("\n%d created/updated, %d already present, %d without standard metrics" - % (created, skipped, empty)) + print( + "\n%d created/updated, %d already present, %d without standard metrics" + % (created, skipped, empty) + ) if not args.apply: print("Dry run -- re-run with --apply.") diff --git a/examples/06_harvest_panels.py b/examples/06_harvest_panels.py index 604871d..13ecdb3 100644 --- a/examples/06_harvest_panels.py +++ b/examples/06_harvest_panels.py @@ -76,7 +76,9 @@ def harvest(api, workspace, projects): def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--workspace", help="workspace to harvest from") - parser.add_argument("--projects", nargs="*", help="limit the scan to these projects") + parser.add_argument( + "--projects", nargs="*", help="limit the scan to these projects" + ) parser.add_argument("--limit", type=int, default=25, help="max projects to scan") parser.add_argument("--into", help="project that should receive the rebuilt view") parser.add_argument("--into-workspace", help="defaults to --workspace") @@ -89,12 +91,17 @@ def main(): api = comet_ml.API() projects = args.projects or api.get_projects(workspace)[: args.limit] - print("Scanning %d project(s) in %s for custom/Python panels...\n" % (len(projects), workspace)) + print( + "Scanning %d project(s) in %s for custom/Python panels...\n" + % (len(projects), workspace) + ) found = harvest(api, workspace, projects) if not found: - print("No custom or Python panels found. Add one from the Panel gallery in the " - "UI, then re-run -- its ids become reusable from the SDK.") + print( + "No custom or Python panels found. Add one from the Panel gallery in the " + "UI, then re-run -- its ids become reusable from the SDK." + ) return for (kind, identifier), (panel, project, view_name) in sorted(found.items()): @@ -109,17 +116,21 @@ def main(): section = Section("Harvested panels", columns=2, height=2) for (kind, identifier), (panel, _, _) in sorted(found.items()): if kind == "custom": - section.add(custom_panel( - identifier, - instance_name=panel.get("instanceName", ""), - default_config=panel.get("defaultConfig", ""), - )) + section.add( + custom_panel( + identifier, + instance_name=panel.get("instanceName", ""), + default_config=panel.get("defaultConfig", ""), + ) + ) else: - section.add(python_panel( - identifier, - revision_id=panel.get("revisionId", ""), - name=panel.get("chartName", ""), - )) + section.add( + python_panel( + identifier, + revision_id=panel.get("revisionId", ""), + name=panel.get("chartName", ""), + ) + ) view = build_view(args.name, sections=[section]) print("\nComposed:") @@ -131,11 +142,15 @@ def main(): target_workspace = args.into_workspace or workspace created = api.create_view(target_workspace, args.into, view) - print("\n[%s] %r in %s/%s" % ("OK" if created else "FAILED", args.name, - target_workspace, args.into)) + print( + "\n[%s] %r in %s/%s" + % ("OK" if created else "FAILED", args.name, target_workspace, args.into) + ) if created and target_workspace != workspace: - print("Note: a panel instance belongs to its own workspace. Copying one " - "across workspaces only works if the panel is shared/public.") + print( + "Note: a panel instance belongs to its own workspace. Copying one " + "across workspaces only works if the panel is shared/public." + ) if __name__ == "__main__": diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index d9268bd..26f0d50 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -339,3 +339,52 @@ def test_get_first_experiment_empty_project(self): if __name__ == "__main__": unittest.main() + + +# -------------------------------------------------------------------------- +# http_error_status +# -------------------------------------------------------------------------- + + +def test_http_error_status_reads_a_real_response(): + from cometx.utils import http_error_status + + exc = Exception("boom") + exc.response = MagicMock(status_code=404) + + assert http_error_status(exc) == 404 + + +def test_http_error_status_ignores_status_text_without_a_response(): + """A status is only trusted from a response, never parsed out of text. + + Callers route on this -- 401/403 is "the key was refused", 404 is "MPM is + not installed or not routed" -- so a status guessed from incidental text + does not degrade to unknown, it asserts something specific and wrong, and + that answer is recorded and rendered. A connection error quoting an inner + frame must stay unknown. + """ + from cometx.utils import http_error_status + + assert http_error_status(OSError("connect failed (status_code: 404)")) is None + assert http_error_status(ValueError("status_code: 401 while parsing")) is None + + +def test_http_error_status_is_none_when_there_is_no_status(): + from cometx.utils import http_error_status + + assert http_error_status(Exception("plain failure")) is None + + no_status = Exception("no response attribute") + no_status.response = None + assert http_error_status(no_status) is None + + +def test_http_error_status_rejects_a_bool_status(): + """`True` is an int in Python; a mock answering True is not a 1xx.""" + from cometx.utils import http_error_status + + exc = Exception("boom") + exc.response = MagicMock(status_code=True) + + assert http_error_status(exc) is None From b619295149ab85c7395516eef3f84e7eee6e2c4a Mon Sep 17 00:00:00 2001 From: Nimrod Lahav Date: Thu, 1 Oct 2026 10:46:22 +0300 Subject: [PATCH 9/9] fix(admin): split the strict and permissive HTTP-status helpers My previous commit made http_error_status refuse to parse a status out of an exception's text, and broke two chargeback tests doing it. The claim that the text branch was unreachable was wrong: it holds for the MPM lookup, whose errors come from the SDK and always carry a response, but not for the chargeback path, which has an explicit test (test_chargeback_status_parsed_from_text_when_no_response) asserting the opposite. One shared helper was serving two callers that want different answers, and I changed it for both. They want different answers because the consequences differ. The MPM lookup records its classification in the mpm_member_lookup KPI and renders it in the HTML, so a guessed 404 asserts "MPM is not installed here" as a result. _chargeback_error_message only chooses the wording of a sentence -- "this needs an admin key" against "that URL is wrong" -- which the reader sees beside the underlying error anyway, so a wrong guess costs a confusing line and nothing more. So: http_error_status stays strict and is what the MPM classification uses, and apparent_http_status keeps the text fallback for callers that only shape a message. Each docstring says which to reach for and why, since the pair is otherwise an invitation to use whichever is nearer. 346 passing, up from 340 on 0afbeda, with the two tests my last commit broke back to green. Verified against the full dependency set this time -- the earlier run was missing reportlab, streamlit and boto3, which hid those two failures behind 200-odd import errors and is why a red build went out. Co-Authored-By: Claude Opus 5 (1M context) --- cometx/cli/admin_growth_report.py | 8 ++++++-- cometx/utils.py | 26 +++++++++++++++++++++++--- tests/unit/test_utils.py | 27 +++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/cometx/cli/admin_growth_report.py b/cometx/cli/admin_growth_report.py index 6a7ea4e..9666f54 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -55,10 +55,10 @@ from cometx.utils import ( InvalidServerURLError, admin_api_url, + apparent_http_status, exception_text, fetch_chargeback_report, format_time_key, - http_error_status, redact_url_userinfo, ) @@ -216,7 +216,11 @@ def _chargeback_error_message(exc) -> str: """Say what actually went wrong fetching the chargeback report, rather than attributing every failure to a non-admin key: a 404 (wrong URL or path prefix) or a 5xx says nothing about the key.""" - status = http_error_status(exc) + # Permissive on purpose: this only chooses the wording of a message a + # human reads, so a status recovered from the exception's text is worth + # having. The MPM classification uses the strict http_error_status, where + # a guess would be recorded as fact. + status = apparent_http_status(exc) response = getattr(exc, "response", None) url = getattr(response, "url", None) # Prefer "HTTP 401: " over the SDK exception's text, which diff --git a/cometx/utils.py b/cometx/utils.py index ba881b5..e7fc84e 100644 --- a/cometx/utils.py +++ b/cometx/utils.py @@ -447,9 +447,8 @@ def http_error_status(exc): is by definition incidental to some inner frame. It had no upside and one failure mode. - `_short_api_error` in admin_growth_report matches the same shape on - purpose and should keep doing so: there the number is only displayed, so a - wrong match is cosmetic rather than a misrouted classification. + `apparent_http_status` below is the permissive counterpart, for callers + that only shape a message someone reads. """ status = getattr(getattr(exc, "response", None), "status_code", None) if isinstance(status, int) and not isinstance(status, bool): @@ -457,6 +456,27 @@ def http_error_status(exc): return None +def apparent_http_status(exc): + """HTTP status of a failed request, falling back to `status_code: NNN` in + the exception's text. + + For callers that only choose the wording of a message a human reads -- + "this needs an admin key" against "that URL is wrong". Guessing is + acceptable there because the reader sees the underlying error alongside + it, and a wrong guess costs a confusing sentence. + + Use `http_error_status` instead whenever the answer is recorded, rendered + as data, or routed on. The distinction is the whole point of having two: + an unknown status that merely reads as unknown is harmless, while one that + becomes `not_found` in a stored result asserts that MPM is not installed. + """ + status = http_error_status(exc) + if status is not None: + return status + match = re.search(r"status_code:\s*(\d+)", exception_text(exc)) + return int(match.group(1)) if match else None + + def redact_url_userinfo(value): """Replace any `user:password@` userinfo in `value` with `***@`. diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 26f0d50..b20a4ac 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -388,3 +388,30 @@ def test_http_error_status_rejects_a_bool_status(): exc.response = MagicMock(status_code=True) assert http_error_status(exc) is None + + +def test_apparent_http_status_may_read_the_text(): + """The permissive counterpart, for message wording only. + + `_chargeback_error_message` needs it: the SDK path it reports on can + surface a failure whose status is only in the text, and the reader sees + the underlying error beside the sentence either way. + """ + from cometx.utils import apparent_http_status + + assert ( + apparent_http_status( + RuntimeError("status_code: 403, body: {'message': 'Forbidden'}") + ) + == 403 + ) + assert apparent_http_status(Exception("nothing to go on")) is None + + +def test_apparent_http_status_still_prefers_a_real_response(): + from cometx.utils import apparent_http_status + + exc = Exception("status_code: 500 somewhere in an inner frame") + exc.response = MagicMock(status_code=404) + + assert apparent_http_status(exc) == 404