diff --git a/README-ADMIN.md b/README-ADMIN.md index d4d31da..c73a95f 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 @@ -172,6 +176,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). 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 @@ -209,9 +214,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 +224,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. 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 @@ -249,7 +255,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` 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/README.md b/README.md index 5c2b761..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). ``` @@ -605,6 +607,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. 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. @@ -628,6 +631,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..d4087b6 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,18 @@ 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. 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( "--chargeback-report", default=None, @@ -916,6 +929,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_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/admin_growth_csv.py b/cometx/cli/admin_growth_csv.py index 9831491..9f4dd20 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_member_lookup=None, ): """Flatten the report's org-level numbers into (name, value, unit) triples. @@ -338,6 +350,26 @@ 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. 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. + # + # `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 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 new file mode 100644 index 0000000..28322dc --- /dev/null +++ b/cometx/cli/admin_growth_mpm.py @@ -0,0 +1,261 @@ +#!/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. 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, + 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 -- +it counts every registry model and overstates MPM adoption. +""" + +from __future__ import annotations + +import concurrent.futures + +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` + 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 _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( + 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) + ] + 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": + """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 +) -> "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, 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: + 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, lookup + + +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` 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( + { + k: v + for k, v in w.items() + if k not in ("mpmEnabled", "monitoredModels") + } + ) + 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..9666f54 100644 --- a/cometx/cli/admin_growth_report.py +++ b/cometx/cli/admin_growth_report.py @@ -24,10 +24,12 @@ import dataclasses import datetime +import json import os 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 +41,7 @@ churn_series, classify_accounts, em_user_breakdown_series, + mpm_reported, opik_user_breakdown_series, parse_users, parse_workspaces, @@ -52,6 +55,7 @@ from cometx.utils import ( InvalidServerURLError, admin_api_url, + apparent_http_status, exception_text, fetch_chargeback_report, format_time_key, @@ -208,6 +212,55 @@ def _fetch_service_accounts(api) -> "set[str] | None": return 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.""" + # 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 + # 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" @@ -334,6 +387,7 @@ def generate_growth_report( no_html=False, chargeback=None, report_date=None, + mpm=False, ): reporter = GrowthReporter( api, @@ -343,6 +397,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 +453,14 @@ 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 + # 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 @@ -444,11 +505,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 @@ -478,9 +535,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_member_lookup = 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 +560,53 @@ 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). + 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 []) + 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, 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( + "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. 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): + 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"]: + note = "MPM checked in %d/%d workspaces" % ( + org["workspaces"] - org["mpm_unchecked"], + org["workspaces"], + ) + 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. @@ -640,21 +747,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 +894,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 +912,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 +948,44 @@ 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". + # 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: + 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 {checked} checked; {unknown_flags} unknown" + if unknown_flags + else f"{pct}% of workspaces" + ), + } + ) + partial = org["mpm_unchecked"] > 0 + lookup_failed = self._mpm_member_lookup not in (None, "ok") + kpis.append( + { + "label": "Monitored models", + "value": ( + f"≥ {org['monitored_models']}" + if partial + else org["monitored_models"] + ), + "sub": ( + self._mpm_note(ws_records) + if partial or lookup_failed + else "MPM (is_monitored)" + ), + } + ) return { "title": "Organization overview (chargeback)", "window_chip": self._window_label(window), @@ -1114,8 +1269,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 +1327,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 +1616,7 @@ def _assemble_report_data( active_window_days, scope=scope, excluded_personal_count=excluded_personal_count, + mpm_member_lookup=self._mpm_member_lookup, ), ) except Exception as exc: diff --git a/cometx/cli/admin_growth_users.py b/cometx/cli/admin_growth_users.py index 7542a3b..9c20350 100644 --- a/cometx/cli/admin_growth_users.py +++ b/cometx/cli/admin_growth_users.py @@ -719,13 +719,63 @@ 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 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.""" + 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) 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): + 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 +788,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 +796,38 @@ 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. 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), "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 + ), + "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), } @@ -770,7 +841,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/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 02d92c2..e7fc84e 100644 --- a/cometx/utils.py +++ b/cometx/utils.py @@ -430,6 +430,53 @@ def exception_text(exc): return type(exc).__name__ +def http_error_status(exc): + """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. + + `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): + return status + 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 `***@`. @@ -491,11 +538,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/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_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..c793abb --- /dev/null +++ b/tests/unit/test_admin_growth_mpm.py @@ -0,0 +1,345 @@ +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, 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() + + +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, _lookup = 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, 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( + "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"])[0] == {"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 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": [ + {"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"] + 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(): + 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"] + + +@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 + + +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]} diff --git a/tests/unit/test_admin_growth_report.py b/tests/unit/test_admin_growth_report.py index dda2a28..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 @@ -2300,3 +2356,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..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 @@ -608,6 +610,11 @@ 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, + "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)) == { @@ -747,3 +754,98 @@ 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) + + +@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 diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index d9268bd..b20a4ac 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -339,3 +339,79 @@ 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 + + +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 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(