Repository navigation
feat(admin): CSV export for growth-report (Glue/Athena/QuickSight) - #27
Conversation
Pure functions that turn parsed UserRecord/WorkspaceRecord objects into Glue-ready CSV rows for three flat tables (users, workspaces, org KPIs). Built from the parsed records rather than the HTML report's display- formatted report_data so numeric columns type correctly in a Glue crawler.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wires the CLI up to the csv_dir/no_html/chargeback kwargs generate_growth_report already supports. Adds argparse validation for --no-html requiring --csv-dir, and a distinct error for an unreadable/malformed --chargeback-report file (separate from the "requires an admin API key" message). Also strengthens test_html_still_written_when_csv_dir_absent to assert byte-identical HTML output with/without --csv-dir under a frozen clock, rather than just file existence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerate the customer-facing sample CSVs to match the final, customer-agreed schema (report_date instead of report_month, no workspace column in growth_users.csv, plus em_last_used_at / opik_last_used_at), remove the superseded Option B/C samples, rewrite docs/csv-samples/README.md around the single agreed schema, and add a CSV export subsection to README-ADMIN.md documenting the --csv-dir / --no-html / --chargeback-report flags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two pre-merge review fixes for the growth-report CSV export. 1. CSV write failures exited 0. The generic `except Exception` handler in the growth-report dispatch printed the error and returned, so an unwritable --csv-dir (or one pointing at a file) reported success while shipping nothing -- the worst failure shape for the monthly scheduled run this feature is built for. Now calls sys.exit(1), matching the sibling GrowthReportError handler, and preserving --debug tracebacks. 2. CSVs were written without an explicit encoding, falling back to the platform locale. Under LANG=C (typical for cron/systemd) a single non-ASCII username raised UnicodeEncodeError mid-write, leaving a truncated CSV for Glue to crawl. Now opens with encoding="utf-8", matching admin_growth_render.py and utils.py. Tests: a regression test driving admin() to a non-zero SystemExit with --csv-dir pointing at a file; a UTF-8 round-trip through DictReader; and a subprocess test that runs the write in a genuinely ASCII locale (the bug cannot be reproduced in-process -- open() resolves its default encoding in C, and PEP 538 coercion turns LANG=C back into UTF-8). A third reported finding (total_users disagreeing with the users table) is deliberately NOT addressed here: the prescribed fix does not achieve its stated goal. See the task report for the analysis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… count `total_users` in growth_org_kpis.csv excludes suspended accounts (it is the licensing/adoption denominator behind active_users_pct), while growth_users.csv carries one row per non-deleted user, suspended included. The two therefore differ for any org with suspended or deleted accounts. This is intentional, so document it rather than changing the metric -- the users table exposes is_suspended, so a dashboard can reproduce either definition from the row data. Adds a caveat to the CSV export section of README-ADMIN.md (matching the existing "no workspace column" caveat) and a two-sentence note to the customer-facing docs/csv-samples/README.md. Also corrects the sample KPI values, which were hand-authored on the naive assumption and contradicted the behavior now documented: with 6 user rows of which 1 is suspended, total_users is 5 (not 6) and active_users_pct is 80.0 (not 66.7). new_users_in_window_pct was likewise wrong at 16.7 -- the real `_window_growth` divides new-in-window by accounts pre-dating the window (1/5 = 20.0), not by the total. The samples now demonstrate the documented rule instead of teaching against it. Regenerated the CSVs and rebuilt the zip. No behavior change: total_users, active_users_60d, active_users_pct and adoption_stats are untouched. Full suite 235 passed, unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/ holds design specs, plans, customer correspondence and customer-deliverable CSV samples — working artifacts, not product code. This is a public repo, so they stay local. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c980a0a to
6532eb2
Compare
The C-locale regression test passed the non-ASCII payload through
`python -c`, so the subprocess had to decode its own command line using
the ASCII locale the test deliberately sets. On Linux CI that fails
before any of our code runs ("Unable to decode the command from the
command line"); macOS happened to tolerate it.
Write the script to a UTF-8 file with a coding declaration and pass the
path instead: argv stays pure ASCII while the source is still read as
UTF-8. Verified still red/green -- reverting the `encoding="utf-8"` fix
in _write_csv reproduces the UnicodeEncodeError the test guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`comet_ml.exceptions.NotFound.__str__` returns None when the 404 body is not JSON -- an HTML error page from a proxy or ingress, for instance. The admin error handler then died with "TypeError: __str__ returned non-string (type NoneType)", replacing the real HTTP error with a traceback from the handler itself. An operator hitting a wrong or unavailable endpoint saw no usable diagnosis. Route the handler through `_exception_text`, which falls back to the exception class name plus the response status code when `str(exc)` is empty or raises. A 404 on the chargeback endpoint now prints "ERROR: NotFound (HTTP 404)". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the helper script by path sets sys.path[0] to the script's own directory (tmp_path) rather than the CWD, so `import cometx` failed on CI with ModuleNotFoundError. It passed locally only because an editable install put the package on sys.path anyway -- exactly the difference between a dev checkout and a clean CI checkout. Pass the repo root explicitly via PYTHONPATH, derived from the imported package rather than hardcoded. Verified against a simulated CI setup (subprocess run from a different cwd with no editable install visible), and still red/green: reverting the encoding="utf-8" fix reproduces the UnicodeEncodeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running against a real deployment surfaced a confusing gap: the total_users KPI read 434 while growth_users.csv had 408 rows, with nothing in the data explaining the difference. The two exclude different populations -- total_users excludes suspended accounts, the users table excludes deleted ones -- so a dashboard showed two tiles disagreeing by 27 with no way to reconcile them. Add a `deleted_at` column (appended last, always empty since deleted users remain filtered out, so no existing sum changes meaning) and a `deleted_users` KPI. The two files now reconcile explicitly: total_users - deleted_users + suspended = users-table row count verified as 434 - 27 + 1 = 408 against the real data. No change to adoption_stats or total_users semantics -- that remains a separate decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Four fixes from the automated review, two of which were real bugs I reproduced before changing anything. 1. Reconciliation broke on deleted+suspended users (logic bug). The documented identity `total_users - deleted_users + suspended` assumed the two exclusions were disjoint. An account carrying BOTH flags is absent from total_users (suspended) AND counted in deleted_users (deleted), so subtracting removed it twice: with 1 live, 1 suspended, 1 deleted and 1 deleted+suspended, the formula predicted 1 row where there were 2. Publish the row count directly as a `users_in_table` KPI instead of asking consumers to derive it, and document that the arithmetic is wrong. Regression test pins the four-way case and asserts the old identity genuinely fails on it. 2. `service_account_source` put a string in the numeric `metric_value` column, which makes a Glue crawler type the whole column as `string` and forces a cast on every SUM/AVG in QuickSight. Add a `metric_text` column for label-unit payloads; `metric_value` is now strictly numeric or empty. 3. `--chargeback-report` opened the snapshot with the platform default encoding, so a file with non-ASCII usernames failed to load under LANG=C -- the same bug already fixed on the CSV write path. 4. Docs claimed `--chargeback-report` avoided the API entirely. It skips the chargeback request only; /api/admin/service-accounts is still queried (verified by instrumenting the call). Corrected both READMEs rather than dropping the call, which is what makes is_service_account authoritative. Samples and zip regenerated against the new schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Python's default float repr switches to exponent form outside roughly 1e-5 .. 1e16, so a workspace with a very small data_mb rendered as "1e-05". Athena's CSV SerDe does not parse that as a double -- the value silently becomes NULL in the dashboard, which is the worst failure shape for this pipeline: no error, just a wrong number. The low bound is reachable in practice (a near-empty workspace logging a few bytes), and the test cluster has 500 empty workspaces. Format floats explicitly with %f, trimming padding zeros, and map nan/inf to an empty field rather than emitting text that would force the whole column to type as string. Ints pass through untouched -- arbitrary precision, never exponential. Real chargeback magnitudes (20545.68, 88834.25, 71200.75) still round-trip exactly. Tests updated to compare rendered values numerically rather than by Python type, since floats are now decimal strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baz dismissed its prior approval because a re-review found new findings.
The previous fix used "%.6f", which avoided scientific notation but quantized ordinary values: 0.1234567 became 0.123457. That traded a silent NULL in Athena for a silently wrong number, which is worse. Five of six sample values lost data. Use repr() whenever it does not produce an exponent -- it is the shortest string that round-trips to the identical float -- and fall back to exact positional expansion via Decimal only when repr goes exponential. Decimal(float) is lossless, so nothing is rounded. Every tested value now round-trips exactly AND contains no exponent, including the real chargeback magnitudes and the pathological ends (1e-07, 1e20, 1/3, 0.1+0.2). Reported by Baz review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dsblank
left a comment
dsblank
left a comment
There was a problem hiding this comment.
Reviewed the full diff on feat/growth-report-csv-export and verified each finding against the branch.
The design decisions in the description hold up — user-grain fact tables, long-format KPIs, and building the CSV from parsed records rather than report_data are all the right calls. Six issues below; (1)–(3) are straightforward, (4)–(5) are a policy call.
1. Workspace numerics bypass the float-formatting guard — cometx/cli/admin_growth_csv.py:174-176
build_workspaces_rows emits w.num_experiments and w.data_mb raw, while the users table (:160-162) and org KPIs (:275) route through _num_or_empty. A workspace with totalSizeInMb = 4e-06 writes 4e-06, which Athena's CSV SerDe reads as NULL; a NaN/inf from the API would be emitted as literal text and force Glue to type the column as string. The module docstring calls out exactly this case ("a near-empty workspace can report a tiny data_mb").
The existing test at tests/unit/test_admin_growth_csv.py:198 asserts float(rows[0][5]) == 12422.75, which passes on a raw float, so it can't catch this.
2. The NotFound.__str__ fix misses the path it was written for — cometx/cli/admin_growth_report.py:215
_short_api_error opens with text = " ".join(str(exc).split()). The chargeback fetch in build() catches its exception and calls this, never reaching admin.py's _exception_text. So for the exact scenario the PR describes — a 404 whose body is an ingress HTML page — str(exc) raises TypeError: __str__ returned non-string from inside the except block, and the real HTTP error is still replaced by a traceback from the handler. _short_api_error should use the same tolerant rendering.
3. Two handlers still use bare str(e) — cometx/cli/admin.py:917 and :923
Same hazard, worse consequence: a broken __str__ raises inside the except, so the sys.exit(1) never runs and the resulting TypeError falls through to the outer handler at :935, which prints and returns 0 — losing both the message and the non-zero exit this PR sets out to guarantee. Both should use _exception_text.
4. Parse failure writes empty CSVs and exits 0 — cometx/cli/admin_growth_report.py:1187-1194
When parse_users/parse_workspaces throws, the code prints a warning and sets people_users = ws_records = []. With --csv-dir that silently writes three header-only CSVs and exits 0 — precisely the "reports success to a scheduler while shipping nothing" mode this PR fixes for write failures. A parse failure should either abort the CSV export or exit non-zero.
5. Same shape for org KPIs — cometx/cli/admin_growth_report.py:1287
Any exception in adoption_stats / _window_growth / classify_accounts / collect_org_kpis leaves self._last_parsed at the earlier (users, ws, []), so growth_org_kpis.csv is written with a header and zero rows, exit 0. A monthly Glue load would quietly ingest an empty KPI partition with only a Warning: line in the log.
6. Dead README link — README-ADMIN.md:322
Links to docs/csv-samples/README.md, but this same PR adds docs/ to .gitignore at line 134 with the comment "never commit". The samples aren't in the repo, so the link is broken for every reader of the public README. Worth noting the blanket docs/ ignore will also silently swallow any future real docs directory here.
All six verified against the branch before changing anything; two were reproduced as live defects. 1. build_workspaces_rows emitted raw floats while the users table and org KPIs both routed through _num_or_empty. A workspace with totalSizeInMb=4e-06 wrote "4e-06" (NULL to Athena) and a NaN/inf wrote literal text (forcing Glue to type the column as string). The existing test used float(...) == ..., which passes on a raw float, so it could not catch this. 2/3. Bare str(e) in three handlers. This is the serious one: a broken __str__ raises INSIDE the except block, so sys.exit(1) never runs, the TypeError falls through to the outer handler, and the process exits 0 -- defeating the exact non-zero-exit guarantee this PR added. Reproduced end-to-end. Every operator-facing render now goes through a single tolerant helper, moved to cometx.utils so _short_api_error can share it (admin.py imports FROM admin_growth_report, so the helper cannot live in admin.py). 4/5. A parse failure or KPI-collection failure degraded to empty lists and wrote three header-only CSVs, exit 0. In Glue that is indistinguishable from "this org has no users", and tells a monthly scheduler the run succeeded. The reporter now records why the export would be empty and generate_growth_report raises instead of publishing it. The HTML path keeps its degrade-and-continue behaviour, which is honest there because a human reads it. 6. README linked to docs/csv-samples/README.md, which this PR gitignores -- broken for every reader of the public README. Replaced with instructions to generate the samples. Also narrowed the ignore from all of docs/ to the two working directories, so a future real docs directory is not silently swallowed. Each fix has a regression test confirmed red before the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @dsblank — all six addressed in 6ee1db3. I verified each against the branch before changing anything; two reproduced as live defects. 1. Workspace numerics bypassed the float guard — confirmed. 2 & 3. Bare Fixed by routing every operator-facing render through one tolerant helper. I moved it to 4 & 5. Empty-CSV-and-exit-0 — agreed, and it's the same failure mode this PR set out to fix for write failures. 6. Dead README link — confirmed broken. Replaced with instructions to generate the samples via Each fix has a regression test I confirmed red before the change — including reverting all four fixes at once to check none of the new tests were vacuous. 252 tests pass. |
Two further Baz findings, both reproduced before changing anything.
1. A --chargeback-report file containing JSON `null` silently fell back
to a live API call. `build()` treats `chargeback is None` as the
fetch-it-live sentinel, and json.load("null") produces exactly that,
so a bad local file was reported as "the chargeback endpoint is
unavailable" -- blaming the server for a client-side problem. Any
non-object payload is now rejected with a message naming the file.
2. A workspace-scoped export was byte-shaped identically to an org-wide
one: same filenames, same headers, only fewer rows and a lower
total_workspaces. Loaded into the same Glue partition it reads as an
organization that shrank overnight, and a scoped run would silently
replace an org-wide snapshot. Adds a `scope` label metric
(`organization`, or `workspaces:a,b`) so the data says which it is.
Rather than reject scoped exports, the capability is kept and made
self-describing -- scoping is legitimate, being indistinguishable is
not.
Note the first regression test I wrote for (1) passed against the
unfixed code: the CLI's broad `except Exception` swallowed the
AssertionError and still exited 1, so it could not tell a rejected
snapshot from a live fetch that merely failed. It now records the call
and asserts no fetch was attempted. All three new tests confirmed red
before their fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit ran `black cometx tests` rather than the files this branch touches, so eleven modules unrelated to the CSV export picked up pure formatting churn and landed on the PR: admin_gpu_app, admin_gpu_report, config, copy, delete_assets, log, rename_duplicates, smoke_test, upload_optimizer_experiments, download_manager and magics. None of it is wrong, but it is noise in a review and would rewrite history other branches are working against. Restored to origin/main. The PR is back to the 8 files it should touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more Baz findings, both consequences of my own previous fixes and
both reproduced before changing anything.
1. `{}` passed the isinstance(dict) snapshot check added last commit.
The chargeback parsers are deliberately permissive -- they return
empty lists rather than raising -- so `_export_blocked` never fired
and the run published three "successful" zero-row CSVs and exited 0.
That is precisely the false-success mode the export guard exists to
prevent, reintroduced through a narrower door. A snapshot must now
carry at least one of `users` or `workspaces`. A one-section snapshot
is still accepted: degraded but real, and rejecting it would break
legitimate input (covered by its own test).
2. The `scope` label serialized the raw CLI request, so it could name a
workspace that does not exist or was dropped by --exclude-personal.
`--workspace team-a ghost` advertised `workspaces:ghost,team-a` while
only team-a had rows, sending a dashboard that filters on `ghost` to
an empty result. `scope` is now derived from the surviving records,
and `scope_requested` records the ask separately when the two differ
-- so a filter that did not land is visible rather than silent.
Both regression tests confirmed red before their fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dsblank
left a comment
dsblank
left a comment
There was a problem hiding this comment.
Re-reviewed at ca02f95. All six findings from my previous review are fixed, each with a targeted regression test. Full unit suite passes locally: 258 passed (was 240).
Verified fixes:
| # | Finding | Fix |
|---|---|---|
| 1 | Workspace numerics bypassed _num_or_empty |
admin_growth_csv.py:181-183 routes all three through it; test_workspace_numerics_go_through_the_float_guard asserts the string form rather than float(...) ==, so it can actually fail |
| 2+3 | Bare str(e) in three handlers |
Helper moved to cometx/utils.py:405 as exception_text, used by _short_api_error and both admin.py handlers. Putting it in utils is the right call given admin.py imports from admin_growth_report |
| 4+5 | Parse / KPI failure wrote header-only CSVs, exit 0 | _export_blocked reason string; generate_growth_report raises instead of publishing. HTML keeps degrade-and-continue, which is correct there |
| 6 | Dead README link | Replaced with generate-your-own instructions; .gitignore narrowed from all of docs/ to the two working directories |
The self-caught findings in 216107f and ca02f95 (JSON null snapshot silently falling back to a live fetch; scoped exports byte-identical to org-wide ones) are good catches, and reverting the stray black sweep across eleven unrelated modules in 7f2f520 was the right call.
Five new items below. Only (A) needs to block.
A. The structural-empty guard covers the snapshot path but not the live one — cometx/utils.py:522, cometx/cli/admin.py:900
ca02f95 rejects a --chargeback-report file carrying neither users nor workspaces, on the reasoning that the parsers are permissive and would otherwise publish a zero-row export. fetch_chargeback_report ends in a bare return response.json(), so the live path gets no equivalent check. Reproduced against this branch with the fetch mocked to return {}:
export_blocked: None
last_parsed lens: [0, 0, 7]
Two header-only tables plus a KPI table of zeros, exit 0 — the exact false-success mode the new guard exists to prevent, reachable through the path that actually runs monthly in production. The check reads more naturally after the fetch in build(), where it covers both sources with one implementation.
B. A scoped run that matches nothing publishes an empty export — cometx/cli/admin_growth_csv.py:277
--workspace ghost against a payload containing only team-a:
blocked: None users/ws: 0 0
KPI: ('scope', '', 'label', 'workspaces:')
KPI: ('scope_requested', '', 'label', 'workspaces:ghost')
scope_requested makes the miss visible in the data, which is the mitigation the commit intends, but the run still exits 0 having published an empty partition. Separately, scope serializes as the bare string workspaces: with an empty list, which every dashboard filter then has to special-case. An empty effective set looks like a natural _export_blocked trigger.
C. _export_blocked is never reset per build — cometx/cli/admin_growth_report.py:413
Set in __init__ and at :1224/:1328, but not cleared at the top of build(). A reused reporter whose first build failed would block every subsequent export. The CLI builds once, so this is latent rather than live — worth a one-line reset for the invariant.
D. Two handlers in the hardened function still render bare {exc} — cometx/cli/admin_growth_report.py:1260 and :1283
The KPI handler eight lines below was converted to _short_api_error, and exception_text's own docstring says "every caller that renders an exception for the operator must go through here." I checked the live risk before raising it: _fetch_service_accounts swallows all exceptions internally and returns None, so an API-borne broken-__str__ cannot actually reach :1283. Consistency, not a defect.
E. Nit — generate_growth_report:354-359 calls reporter.export_blocked() twice; assign it to a local once.
Adjacent and out of scope for this PR: admin.py:663, :784 and :854 still use bare str(e) in other subcommands' handlers — same hazard class, reasonable as follow-up.
Requesting changes on (A), since it is the same class of bug as the fix that motivated the guard, on the more commonly exercised path. (B) is a short addition to the same check. (C)–(E) can ride along or wait.
Doug's re-review. (A) blocks; implementing (B)-(E) alongside since they are all in the same function. A. The structural-empty guard I added in ca02f95 covered only the --chargeback-report path. `fetch_chargeback_report` ends in a bare `return response.json()`, so a live `{}` response reached the permissive parsers, left _export_blocked unset, and published two header-only tables plus a zeros KPI table, exit 0. That is the same false-success mode the guard exists to prevent, on the path that actually runs monthly in production -- I had hardened the occasional path and left the common one open. Reproduced before fixing. Moved the check into build() after the fetch, per Doug's suggestion, so one implementation covers both sources. B. `--workspace ghost` against a payload without it exported an empty partition at exit 0, and serialized `scope` as the bare string `workspaces:` that every dashboard filter would have to special-case. An empty effective scope now blocks. Checked after assembly, since only then is it known which workspaces survived scoping and --exclude-personal. A scope that does match still exports (tested). C. _export_blocked now resets at the top of build(); a reused reporter whose first build failed would otherwise block every later export. Latent, not live -- the CLI builds once -- but the invariant is cheap. D. Routed the leaderboards and personal-vs-service warnings through _short_api_error, and the "could not reach the chargeback endpoint" error through exception_text. Doug correctly notes the API-borne case cannot reach the first two today, but the endpoint error is exactly the path this hardening was written for. E. Assign reporter.export_blocked() to a local instead of calling twice. Regression tests for A, B and C confirmed red before their fix; the positive cases (matching scope, org-wide) are tested too, so the new blocks cannot fire on a legitimate run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks Doug — all five implemented in 9276ed3. I reproduced (A) and (B) against the branch before changing anything; both were exactly as described. A — blocking finding. You're right, and the framing matters: I hardened the Took your suggestion and moved the check into B. Also confirmed, including the C. Added the reset at the top of D. Routed the leaderboards and personal-vs-service warnings through E. Done. Regression tests for A, B and C confirmed red before their fixes. 262 tests pass. On the adjacent items — |
`bool` subclasses `int`, so `_num_or_empty(True)` passed the numeric path and wrote the literal `True` into experiment_count / data_logged_mb / opik_span_count. A Glue crawler then types the whole column as `string` and every aggregation in QuickSight needs a cast -- the same failure class as the scientific-notation and metric_value bugs earlier in this branch. Booleans now render as an empty field. Coercing to 1/0 would be worse: it invents a count the source never reported, and "no usable value" is what an empty field already means everywhere else in this export. Requires a malformed upstream payload, so this is defensive rather than a live defect. The genuine boolean columns (is_suspended, is_service_account) are unaffected -- they emit 0/1 through explicit conditionals, never through _num_or_empty -- and the test asserts that so a future change here cannot silently break them. Reported by Baz review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two cosmetic findings from Baz; no behaviour change. `export_blocked()` / `_export_blocked` read as booleans but store and return an explanatory string, so `if reporter.export_blocked():` looked like a flag check while the value was a sentence. Renamed to `export_block_reason` / `_export_block_reason`, including the local at the call site, which had the same problem. Also reworded the boolean-guard docstring: "rejected to empty" -> "converted to empty fields". 263 tests pass; the rename is mechanical and covered by the existing export-block tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the CLI locally leaves growth_report.html, the three growth_*.csv files and comet-chargeback-report.json in the working tree. These hold real usernames, emails and usage figures from whatever deployment they were generated against -- the ones sitting here now carry live @comet.com addresses from the EKS test run -- and this is a public repo. Nothing was committed, but they were untracked-and-unignored, so a `git add -A` would have swept them in. Matched by filename rather than directory so they are caught wherever --csv-dir points, not just ./out. The synthetic samples in docs/csv-samples/ are unaffected: that directory is already ignored, and the generator still reproduces them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review is in. It confirms 8 findings; I spot-checked the four highest-severity ones against the code directly. Correctness
Robustness / operability
Verified clean: row/header arity on all three tables, Decimal expansion both exponent directions and -0.0, NaN/inf, bool rejection, exception_text fallback, sys.exit(1) not swallowed by the except Exception handlers, collect_org_kpis arg order, scope/scope_requested mismatch, keyword-only defaults preserving existing callers. 105 related tests pass. Findings 1–3 look like the ones worth fixing before merge — all three undermine guards this PR added deliberately. |
missing section Doug's findings 1-3. 1. The scope metric only branched on --workspace, so a run using the pre-existing --exclude-personal dropped workspaces and still labelled itself `organization` -- provenance byte-identical to a genuine org-wide run, which would overwrite the org-wide Glue partition. That is the exact failure the metric was added to prevent; it just did not know about the other way the data gets filtered. Now labels `organization_excluding_personal` and emits an `excluded_personal_workspaces` count. Derived from how many workspaces were actually dropped, not from the flags, so a pattern that matches nothing still reads `organization` (tested). 2. The emptiness guard read `not users and not workspaces`, firing only when BOTH were absent, while its message said "neither". A workspaces-only payload exported a header-only users table with the total_users / active_users_* / new_users_* KPIs silently missing. Either section missing now blocks. The test asserting the old behaviour was mine and was wrong -- inverted. Also removed the duplicate check in admin.py: build() applies the same rule to both the snapshot and the live fetch, and having two implementations is what let them disagree. 3. Declined, and removed the guard behind it instead. Reaching that AttributeError needs our own admin endpoint to return a JSON array; the response shape is a contract we own, not untrusted input. The `isinstance(chargeback, dict)` half was over-defense from an earlier round -- dropping it makes the finding moot and removes a branch that cannot execute in production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks Doug — 1 and 2 fixed in 66c0e7a, 3 handled by removing the guard rather than extending it. Reproduced all three first. 1 — fixed. Confirmed: Now emits 2 — fixed. My bug: I wrote I also deleted the duplicate check in 3 — declining, and removing the guard behind it. Reaching that Same reasoning for the On 4–8: all fair, and I'd like to take them as follow-ups rather than another round here. 4 is the closest to earning its place; 5–8 are robustness and operability polish on a path that has now had eleven review rounds without a real Glue load behind it. This PR started as "a flag that writes the CSVs" and the feature itself is ~390 lines; most of what has accumulated since is hardening against shapes our own API does not produce. I'd rather ship, run a real monthly export into Glue, and let that surface what actually matters. 265 tests pass, 8 files. Happy to do any of 4–8 now if you'd rather they land together. |
The filter trimmed only the workspace list and left the user roster whole, so a single `scope` label covered two different populations: on a 2-workspace payload the workspace table reported 5 experiments while the user-derived KPIs reported 20, and users belonging solely to an excluded workspace still appeared in growth_users.csv. Labelling the user table "roster-wide" was the alternative, but that leaves a dashboard summing incomparable numbers under one partition. Narrowing is also what --workspace already does. Reuses `_scope_chargeback`, the helper that implements exactly this rule for an explicit --workspace selection, rather than growing a second implementation that could drift. The two filters now produce identical output for an equivalent selection, which is asserted directly. Reported by Baz review. Regression tests confirmed red beforehand; the unfiltered path is tested too, so the narrowing cannot fire when no filter is active. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The leading slash anchored the pattern to the repository root, so an HTML report written elsewhere via --output stayed unignored. These files carry real usernames, emails and usage figures, and this is a public repo, so the pattern should match at any depth -- as the three growth_*.csv patterns beside it already do. Reported by Baz review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nal runs Baz findings 18 and 19, both confirmed against HEAD, plus the gaps a reader hits next. 1. The post-assembly emptiness guard only ran when an explicit --workspace scope was given, so an --exclude-personal pattern matching every workspace wrote header-only CSVs and exited 0 -- exactly the "an org with no users" partition the guard exists to prevent, reached by the other filter. Both filters are now checked. Users are checked alongside workspaces, for the same reason the structural guard blocks on EITHER missing section: both filters narrow the roster to members of the surviving workspaces, so surviving workspaces with no members still ship a header-only growth_users.csv and drop every user-derived KPI. The HTML-only degradation path is preserved and tested: an empty section is honest in HTML, it is only the CSV partition that must not ship. Passing --csv-dir is what turns an empty result into an error. 2. _scope_label only considered an explicit scope, so an unscoped --exclude-personal run labelled its HTML `Org-wide` while the CSV called the same run `organization_excluding_personal`. The filter narrows users as well as workspaces, so the report is a subset, not the org -- and the header's own workspace/user counts were already post-exclusion. Driven by the count actually dropped, not by the flag, matching how collect_org_kpis decides the same thing; a test pins both provenances to one run so they cannot drift apart again. Found while verifying the above: 3. write_growth_csvs wrote the three tables in place, one after another, so a failure partway through (a full disk, a read-only file from a previous run) left one fresh file beside two stale ones. The upload step syncs the directory, not the path list returned, and nothing about the files says the run failed -- worse than the header-only export, which at least fails loudly. They are staged and moved into place only once all three are written. 4. build_org_kpi_rows wrote metric_value raw while the users and workspaces builders guard their numeric columns. It is the single writer of that column, so the guard belongs there; idempotent, and a no-op on the report's own path. 5. README documented neither the `organization_excluding_personal` scope nor `excluded_personal_workspaces` (added last commit), and still promised header-only files for an empty run -- which the block reasons have contradicted since they were introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit hardened growth-report's exit code, but `admin()` had
14 more `print("ERROR: ..."); return` paths -- across usage-report,
gpu-report and optimizer-report -- plus an outer handler that caught
everything else and returned 0. All of them reported success while
producing nothing.
That is the worst shape for the unattended runs this command is built
for: a monthly cron or CI job sees exit 0, records the run as done, and
the pipeline downstream silently keeps whatever stale data it had. It is
the same failure the CSV write-failure fix addressed, left in place
everywhere except the one path that happened to get reviewed.
- All 14 error returns now `sys.exit(1)`. Each one already printed
`ERROR:` first; none were legitimate non-error early returns.
- The outer `except Exception` exits 1 rather than returning.
- CONTROL+C exits 130 (128 + SIGINT, the shell convention) rather than 0.
An interrupted run is not a successful one.
- Those handlers now render through `exception_text` like the
growth-report path does. With a `sys.exit(1)` after the print, a broken
`__str__` (comet_ml's `NotFound` returns None for a non-JSON 404 body)
would raise inside the handler and skip the exit entirely -- restoring
the exact bug being fixed. Tested.
`--debug` still re-raises for a full traceback, and the interpreter exits
non-zero on the propagated exception, so that path was already correct.
New tests pin the exit code for each subcommand's failure paths; 9 of the
10 fail against the previous commit. The tenth pins a healthy run to 0,
so the guard cannot overshoot into failing a successful run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baz findings on the staging I added in 7e42a95. 1. Predictable `<name>.tmp` staged through `open(..., "w")` (high). `--csv-dir` is an operator-supplied path that may be world-writable, so a pre-planted symlink at the fixed name would be followed and truncate whatever it pointed at, and two runs into one directory would write and clean up each other's staged data under the same name. Staging now goes through `tempfile.mkstemp`, which is O_CREAT|O_EXCL and unpredictable. mkstemp creates 0600; these are published data files whose upload step may run as another user, so the mode is restored to what a plain `open()` would have produced rather than silently narrowed. 2. Mixed generations across the commit (high). Partially addressed, and the overclaim removed. Fixed: a failure partway through no longer leaves earlier replacements standing. Each displaced file is held aside until all three have landed, and any failure rolls the directory back to the generation it held on entry. Not fixed: POSIX has no atomic multi-file rename, so a reader walking the directory *during* the commit can still observe a mix. Closing that means publishing into a versioned directory and swapping a symlink, which changes the output layout consumers already read -- a spec change, not a bug fix, and one for the customer to weigh. The window is the microseconds between three renames, against a consumer that uploads after the process exits and checks the exit code; before this PR the same mix was visible for the entire duration of the write. The comment claiming a reader "sees either the whole previous set or the whole new one" was wrong and is now an explicit statement of the limit. 3. `excluded_personal` read as a boolean while holding a count. Renamed to `excluded_personal_count` throughout, and the label fragment built from it is now `excluded_label` so the two are not confusable. Tests: a planted symlink at the old predictable name is not followed; concurrent staging gets distinct names; a failed commit restores all three previous files; published files keep the mode a plain open() would give. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Net -21 lines in admin_growth_csv.py. Baz found a real bug in the rollback machinery: if the inner restore `os.replace(backup, path)` failed, the backup was never recorded in `committed`, so cleanup could not remove it AND the destination was left missing. Same gap when the initial displace failed, leaving an untracked empty .bak. Rather than add more bookkeeping, the bookkeeping is gone. That machinery was speculative hardening I added in 7e42a95 -- not a review finding, not a customer requirement -- and it has produced a genuine bug in each round since. It guarded a mid-commit `os.replace` failure: an I/O error within one directory, after every byte is already written, on a run that exits non-zero either way. The cost of covering it exceeded the failure. Kept, because these were real and are cheap: - `mkstemp` staging. O_CREAT|O_EXCL, unpredictable name: a pre-planted symlink at a fixed `<name>.tmp` is never opened, and concurrent runs into one directory no longer share staged names. The realistic failure -- a write dying partway -- still publishes nothing at all. - The mode restore now uses `os.fchmod` on the descriptor rather than `os.chmod` on the path, so there is no window between creating the file and setting its mode in which the name could be pointed elsewhere. This is the actionable half of the "attacker controls chmod" finding; the publish itself is `rename()`, which takes a pathname and has no by-descriptor form, and an attacker who can write to `--csv-dir` can rewrite the published CSV a millisecond later regardless. - A chmod failure now warns instead of passing silently. It stays non-fatal (a filesystem without POSIX modes is no reason to fail a good export), but a file left at 0600 that an uploader cannot read is no longer invisible. The comment and README now state the limit rather than implying a guarantee: the commit is not a transaction, a reader mid-commit can see a mixed set, and the exit code is the signal to trust. A test pins that limit so nobody re-derives the guarantee from the code's shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mode was widened right after `mkstemp` and before any rows were written, so a staged file holding chargeback data was group/world readable for the whole duration of the write, and a `.tmp` left by a killed run stayed readable. Widening now happens after the rows are written, with an explicit flush first so nothing is still sitting in Python's buffer when the file becomes readable. Not applied to the published path, which is what the finding suggested: `os.chmod` by name after `os.replace` would reintroduce the exact window removed in 6f937c5, where the name can be pointed elsewhere between publishing and setting the mode. The mode survives the rename, so setting it on the descriptor beforehand is equivalent and has no such window. Finished-but-unpublished temporaries do sit at the published mode while later tables are staged. That is deliberate: they hold complete tables, identical in content and mode to the files they are about to become. Test observes the mode of the descriptor actually being written -- not a directory scan, which would also catch those completed temps -- and fails against the previous ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
User description
Why
A customer runs
cometx admin growth-reportmonthly and feeds the results into a dashboard: CSV → S3 → AWS Glue tables → Athena → QuickSight. Today the command only emits a self-contained HTML page, which that pipeline can't consume. They asked for CSV output of the aggregated data, plus a definite schema so Glue tables can be created against it once and not break on later runs.What this adds
Three new flags on
growth-report:--csv-dir DIRDIR--no-html--csv-dirfor CSV-only runs)--chargeback-report FILEOutput
growth_users.csvgrowth_workspaces.csvgrowth_org_kpis.csvDesign decisions (agreed with the customer)
Full-grain fact tables, not the HTML's derived views. The report's leaderboards and KPIs are top-N slices and sums of these same records. Exporting the full grain lets QuickSight reproduce every chart and answer questions the HTML doesn't, without a new cometx release per requested cut.
No
workspacecolumn on the users table. Chargeback reportsexperiment_count/data_logged_mb/opik_span_countper user, not per (user, workspace). A row per (user, workspace) would repeat each user's totals and makeSUM()over-count. One row per user means a plainSUMis correct with noDISTINCT. Exact per-workspace totals live ingrowth_workspaces.csv. Documented trade-off: per-workspace user breakdowns aren't answerable from this export.Org KPIs are long-format (
metric_name/metric_value) so new metrics arrive as new rows — the Glue schema never changes and existing partitions stay readable.CSV is built from the parsed records, never from
report_data. This is the load-bearing decision:report_dataholds display-formatted strings (_num()inserts thousands separators, rates carry%), which would make a Glue crawler type those columns asstringand silently break aggregation in the dashboard.Validated against a real deployment
Run against a live self-hosted EKS cluster (408 users, 523 workspaces):
%suffixes, or other Glue-hostile formatting in any value.Compatibility
Existing behavior is unchanged when the new flags aren't passed — enforced by a test asserting the HTML is byte-identical (frozen clock, SHA-256) with and without
--csv-dir.Testing
240 unit tests pass (baseline was 207). Coverage includes: one row per multi-workspace user, deleted excluded / suspended flagged, epoch-ms → ISO dates,
None→ empty field (distinct from a real0), no separators or%in any value, header written even with zero rows, non-ASCII round-trip under a C locale, and non-zero exit on write failure.Notes for the reviewer
Two pre-existing issues fixed here, both found while running against the real cluster:
--csv-dirprinted an error but reported success — the worst shape for a scheduled monthly job, which would report success while shipping nothing. Now exits non-zero.__str__.comet_ml.exceptions.NotFound.__str__returnsNonewhen the 404 body isn't JSON (an HTML page from a proxy/ingress), sostr(exc)raisedTypeErrorand replaced the real HTTP error with a traceback from the handler itself. A 404 now printsERROR: NotFound (HTTP 404).Two documented quirks a reviewer will notice in the data:
total_userswon't equal the users-table row count. The KPI excludes suspended users; the table excludes deleted ones. On the real deployment: 434 vs 408. Adeleted_usersKPI makes this reconcile explicitly (434 − 27 + 1 = 408). This is pre-existing behavior —adoption_statsis untouched by this PR and the HTML report has shown 434 all along. Changing the metric's semantics is deliberately deferred as a separate decision.Implementation notes:
admin_growth_csv.pydeliberately does not import fromadmin_growth_report.py(that module imports fromadmin_growth_users.py; importing back would be circular). It takes already-parsed records as arguments.report_dateis the UTC run date, injectable as a parameter for tests but intentionally not exposed as a CLI flag.deleted_atis emitted as a column but is always empty, since deleted users remain filtered out. It exists so the column is typed for a Glue crawler and the schema needn't change if that policy is revisited.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR admin_("admin"):::modified generate_growth_report_("generate_growth_report"):::modified GrowthReporter_build_("GrowthReporter.build"):::modified ADMIN_API_("ADMIN_API"):::modified GrowthReporter_assemble_report_data_("GrowthReporter._assemble_report_data"):::modified collect_org_kpis_("collect_org_kpis"):::added write_growth_csvs_("write_growth_csvs"):::added stage_csv_("_stage_csv"):::added short_api_error_("_short_api_error"):::modified exception_text_("exception_text"):::added admin_ -- "Adds CSV-only, HTML suppression, and preloaded chargeback report options." --> generate_growth_report_ generate_growth_report_ -- "Passes optional chargeback payload and captures parsed export data." --> GrowthReporter_build_ GrowthReporter_build_ -- "Skips API fetch for local payloads and improves endpoint error reporting." --> ADMIN_API_ GrowthReporter_build_ -- "Adds exclusion counts and records parsed users, workspaces, and KPIs." --> GrowthReporter_assemble_report_data_ GrowthReporter_assemble_report_data_ -- "Exports normalized user, workspace, adoption, growth, and account KPIs." --> collect_org_kpis_ generate_growth_report_ -- "Writes three Glue-ready CSV fact tables with report dates." --> write_growth_csvs_ write_growth_csvs_ -- "Stages complete tables privately before atomic publication." --> stage_csv_ short_api_error_ -- "Prevents broken exception stringification from masking HTTP errors." --> exception_text_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxAdd Glue-ready CSV fact-table export to
growth-report, supporting users, workspaces, long-format KPIs, local chargeback snapshots, CSV-only execution, schema-safe numeric normalization, scope provenance, and staged publication while preserving existing HTML output. Harden admin command error reporting and exit statuses across API, parsing, output, interruption, and malformed-exception paths, with documentation and extensive regression coverage.__str__is broken while retaining HTML-only degradation when CSV export is not requested.Modified files (6)
Latest Contributors(2)
Modified files (6)
Latest Contributors(2)
Modified files (1)
Latest Contributors(2)