Repository navigation
[OPIK-8411] MPM presence in growth-report via --mpm (OPIK-8411) - #28
Conversation
Chargeback carries no MPM data, so `--mpm` collects it client-side from existing endpoints and merges it into each chargeback workspace: - /api/mpm/v3/workspaces answers, in one call, every workspace the API key's user belongs to. - The REST v2 model registry covers the rest: list each workspace's models, then read each model's monitored flag (parallel, 8 workers). Both apply the same is_monitored predicate server-side. A workspace whose lookup fails is reported as unknown, never as zero. Adds MPM workspaces / Monitored models KPIs, an MPM models column in the by-workspace table, an MPM leaderboard, mpm_enabled / num_monitored_models columns in growth_workspaces.csv, and mpm_workspaces / total_monitored_models / mpm_workspaces_unchecked org KPIs. Off by default because of the per-model requests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
| Topic | Details | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| MPM growth reporting | Collect MPM presence through fetch_mpm_presence, merge monitored models into chargeback workspace records, preserve unknown and partial results, and expose the data through report KPIs, tables, leaderboards, CSV exports, CLI help, documentation, and regression tests.Modified files (11)
Latest Contributors(2)
| |||||||||
| Admin API reliability | Correct admin_api_url handling for SDK /clientlib roots and distinguish authentication failures, missing endpoints, malformed responses, and server errors so chargeback and MPM workflows produce accurate, safe diagnostics across deployment prefixes.Modified files (5)
Latest Contributors(2)
| |||||||||
| Codebase formatting | Apply formatting and import-order cleanup to administrative reports, copy and download utilities, duplicate renaming, and example workflows without changing their behavior, improving consistency and maintainability across the broader CLI and sample surface.Modified files (12)
Latest Contributors(2)
|
The SDK's comet.url_override ends in /clientlib/, and MPM is served next to it. Under it, the call 404'd and --mpm silently fell back to the slower per-model registry path. Found running against mpm-demo.dev.comet.com, where /clientlib/api/mpm/v3/workspaces is 404 and /api/mpm/v3/workspaces is 200. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
For a workspace the user isn't a member of, the registry returns only public models to a user who isn't an organization admin, with no error. Private monitored models are then missed silently and the workspace still counts as checked. This rule differs from chargeback's (server admin list, or org admin on-prem), so a key that passes chargeback may still undercount. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Every chargeback failure used to be reported as "requires an admin API key", including a 404 from a wrong URL or path prefix. Now: - 401/403: needs an admin user's key (a server admin, or an organization admin on self-hosted); workspace roles such as Manage don't count. - 404: endpoint not found at <url>; check the server URL. - non-JSON response: likely an SSO or proxy login page. - other failures: could not fetch, without blaming the key. Show "HTTP <status>: <server message>" instead of the SDK exception text, which included part of the API key and a truncated URL. Document who counts as an admin in both READMEs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…er it admin_api_url kept every path prefix of the base URL, including the /clientlib segment the SDK's comet.url_override ends with. That segment is the SDK's own API root, and the admin and MPM APIs are served beside it, so chargeback, service-accounts and migrate-users called /clientlib/api/admin/... and got a 404 on servers such as mpm-demo, where /api/admin/... answers. Drop a trailing /clientlib segment and keep any real deployment prefix in front of it (/comet/clientlib -> /comet), the way smoke_test already does. The MPM-only workaround in admin_growth_mpm now just uses the shared helper. The old test that asserted the /clientlib/api/admin URL is replaced by cases for the SDK base, a deployment prefix, a prefix-only base, and a lookalike segment. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…omplete Addresses the review comments on #28: - MPM workspaces card: the percentage is over the workspaces actually checked ("100% of 1 checked; 1 unknown"), not over all workspaces. - Monitored models: a total missing any workspace is shown as a lower bound ("≥ N"). workspace_org_totals now returns mpm_checked and mpm_unchecked, derived from the records, so partial coverage is labeled the same way for --chargeback-report files. The separate _mpm_status run state is removed. - mpm_workspaces_unchecked counts workspaces with an unknown model count, and is emitted even when every lookup failed. - apply_mpm_presence removes existing mpmEnabled / monitoredModels from a workspace whose lookup failed, instead of passing stale values through. - A fractional or negative monitoredModels count is treated as unknown instead of being truncated. - README-ADMIN: fix a typo, and say the CSV totals are exact only when mpm_workspaces_unchecked is 0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
SageMaker: the auth works and needs nothing here, but a failure would be invisibleChecked whether The chain: The explicit The hardcoded path is correct, and that is worth recording because it reads as a bug. Server side, One thing I could not verify: the The one change I would ask for
That matters because of the second caveat this file already documents: on a non-member workspace, a caller who is not an org admin sees only public models, with no error. So if the MPM endpoint fails and the key is not an org admin, the two silent paths compose into a confident-looking low number rather than an unknown. This is the failure mode the module sets out to prevent — "a false zero would read as a workspace that stopped using MPM" — and the fallback reintroduces it one level up. Suggest logging the exception at debug or warning, and distinguishing 401/403 from the rest, so a deployment where this never worked is visibly different from one where MPM is simply not installed. Worth confirming on a deployment with |
Any failure of the member lookup used to be swallowed, with a silent fall back to the registry. That included 401/403 (e.g. under SageMaker auth) and proxy routing misses. A refused key that is also not an org admin then got a confident-looking low count instead of a visible problem, because the registry hides private models in non-member workspaces. The lookup result is now classified: ok, refused (401/403), not_found (404: MPM not installed or not routed), or error (anything else, including an unexpected response shape). Each failure prints a warning naming the URL and status; for refused, the warning also says the registry fallback may undercount. The result is recorded as an mpm_member_lookup label KPI in growth_org_kpis.csv, and in the HTML notes, including the Monitored models card even when every workspace was checked. http_error_status moves to cometx.utils so the MPM module can share it with the chargeback error messages without a circular import. The module docstring records why the /api/mpm/v3/workspaces path is fixed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for tracing the SageMaker path. Agreed on the change you asked for; it's in 0afbeda. The
The registry fallback still runs in every failure case, but the result is recorded:
I also recorded in the module docstring why the Tested:
I kept the explicit |
Baz dismissed its prior approval because a re-review found new findings.
Baz's finding on the classification added in 0afbeda, and it is right. http_error_status fell back to parsing `status_code: NNN` out of the exception's text when there was no response. Callers route on the result -- 401/403 means the key was refused, 404 means MPM is not installed or not routed -- so a status recovered from incidental text does not degrade to "unknown". It asserts something specific and wrong, which is then recorded in the mpm_member_lookup KPI and rendered in the HTML. A connection reset quoting an inner frame became "MPM is not installed here". That is the failure the classification was added to prevent, so the fallback defeated the commit it shipped in. Nothing is lost by refusing to guess, because the branch was unreachable for the errors it was meant to serve. Every HTTP failure the SDK raises carries a response: CometRestApiException.__init__ always assigns one, and NotFound and Unauthorized both subclass it, so the first branch returns. Its own __str__ renders "failed with status code 404" -- no colon -- so the pattern would not have matched that text even if it were reached. The branch could only ever fire for a non-HTTP exception, where any match is incidental by definition. One failure mode, no upside. _short_api_error in admin_growth_report matches the same shape and keeps it deliberately: there the number is only displayed, so a wrong match is cosmetic rather than a misrouted classification. The docstring says so, since the two now differ on purpose. Four tests, including the connection-error case that regressed it. Verified as guards: restoring the fallback fails the text case. pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
My previous commit made http_error_status refuse to parse a status out of an exception's text, and broke two chargeback tests doing it. The claim that the text branch was unreachable was wrong: it holds for the MPM lookup, whose errors come from the SDK and always carry a response, but not for the chargeback path, which has an explicit test (test_chargeback_status_parsed_from_text_when_no_response) asserting the opposite. One shared helper was serving two callers that want different answers, and I changed it for both. They want different answers because the consequences differ. The MPM lookup records its classification in the mpm_member_lookup KPI and renders it in the HTML, so a guessed 404 asserts "MPM is not installed here" as a result. _chargeback_error_message only chooses the wording of a sentence -- "this needs an admin key" against "that URL is wrong" -- which the reader sees beside the underlying error anyway, so a wrong guess costs a confusing line and nothing more. So: http_error_status stays strict and is what the MPM classification uses, and apparent_http_status keeps the text fallback for callers that only shape a message. Each docstring says which to reach for and why, since the pair is otherwise an invitation to use whichever is nearer. 346 passing, up from 340 on 0afbeda, with the two tests my last commit broke back to green. Verified against the full dependency set this time -- the earlier run was missing reportlab, streamlit and boto3, which hid those two failures behind 200-odd import errors and is why a red build went out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds MPM adoption to
cometx admin growth-reportfor OPIK-8411: which workspaces use MPM and how many models each one monitors, next to the existing EM and Opik stats. It also fixes the admin API URL and the chargeback error messages, which were blocking growth-report on servers such as mpm-demo.How
--mpmcollects the dataThe chargeback report has no MPM data, and the backend is out of scope, so
--mpmgathers it from existing endpoints. It mergesmpmEnabled/monitoredModelsinto each chargeback workspace before the report is built:/api/mpm/v3/workspaces: one call covering every workspace the API key's user belongs to. The path is deliberately fixed: it's a backend-react route, so it resolves the same way on Cloud and on chart deployments.registry-model?workspaceName=lists the models, thenregistry-model/detailsgives each model's monitored flag. This is one request per model, so the calls run in parallel (8 workers).Both sources apply the same filter server-side:
is_monitored, not pipeline-generated, noref_production_model_id, not deleted (RegistryQueries.getMonitoredRegistryModelsForWorkspacesSql). When workspaces are named on the command line, only those are checked.--mpmis off by default because of the per-model requests.Nothing unknown is reported as zero or as complete:
mpm/v3/workspacescall is classified, not swallowed (review feedback). The results arerefused(401/403),not_found(404: MPM not installed or not routed) anderror. Each prints a warning, is shown in the HTML notes, and is recorded in the CSV.refusedalso warns that counts may be low (see Access below).nb_models_registeredis not used, because it counts every registry model (the ticket's caveat).Output
growth_workspaces.csv: newmpm_enabled(1/0, soSUM()counts MPM workspaces) andnum_monitored_modelscolumns, appended at the end so existing columns keep their order. Both are empty when that workspace's MPM status is unknown.growth_org_kpis.csv:mpm_workspacesandtotal_monitored_models: exact only whenmpm_workspaces_uncheckedis 0.mpm_workspaces_unchecked: workspaces whose model count is unknown. Emitted even when every lookup fails.mpm_member_lookup: a label metric (ok/refused/not_found/error), so a dashboard can tell "MPM absent" from "auth refused".Access: who counts as an admin
The two parts of this report use different permission checks:
@AdminApiKey,ApiKeyAuthFilter): the key's user must be in the server's admin user list or, on self-hosted installs, an organization admin. Workspace roles such as Manage don't count.--mpmregistry route: organization admins can read every workspace. For a workspace the user isn't a member of, a user who isn't an organization admin gets only its public models, with no error. Private monitored models are then missed. This is documented in--help, both READMEs, and the module docstring.mpm_member_lookup=refusedflags the most likely case, but the code can't detect it directly.Also fixed
Admin URLs under the SDK's
/clientlibroot.admin_api_urlkept the/clientlibsegment that the SDK'scomet.url_overrideends with. So chargeback, service-accounts, migrate-users and the MPM call all went to/clientlib/api/..., which is a 404 on mpm-demo. It now drops a trailing/clientlibsegment and keeps any deployment prefix in front of it (/comet/clientlibbecomes/comet), matchingsmoke_test.Accurate chargeback errors. Every chargeback failure used to print "requires an admin API key", including that 404. The message now depends on the failure:
<url>; check the server URL (not an API-key problem)The detail shown is
HTTP <status>: <server message>. Before, it was the SDK exception text, which included the first 5 characters of the API key and a truncated URL.Limitations
--csv-direxports.Testing
340 unit tests pass, and pre-commit hooks are clean on the changed files.
tests/unit/test_admin_growth_mpm.pycovers:--mpmon and off throughGrowthReporter.buildOther test files cover the URL handling and each chargeback error message.
Live on mpm-demo.dev.comet.com (
comet.mpm.enabled), full run with an admin key (growth-report --mpm --csv-dir, no URL override): exit 0 in about 4s.mpm/v3/workspaces, 9 via the registry,mpm_member_lookup=ok,mpm_workspaces_unchecked=0.monitored, notisMonitored. Both are accepted./clientlibURL.Not tested live: the
refusedandnot_foundlookup results (unit tests only), and a SageMaker deployment. Review feedback traced the SageMaker auth path throughapi._clientand found no code needed.🤖 Generated with Claude Code