Skip to content

[SC-18115] Add lightweight metrics SDK - #556

Open
cachafla wants to merge 5 commits into
mainfrom
cachafla/sc-18115/lightweight-metrics-sdk
Open

cachafla wants to merge 5 commits into
mainfrom
cachafla/sc-18115/lightweight-metrics-sdk

Conversation

@cachafla

@cachafla cachafla commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

What and why?

Adds a dependency-light validmind-metrics SDK and shared validmind-tracking-core package for applications that only need to send metrics. The existing library now uses the shared synchronous metric transport, while its async metric API offloads the blocking request instead of creating an aiohttp session. This removes nested event-loop and closed-session failures when metrics are sent from HTTP handlers, while preserving the full library's other async operations.

Installation

Users who need the full ML and testing library continue to install:

pip install validmind

Users who only need to send ValidMind metrics can install the lightweight SDK:

pip install validmind-metrics

The metrics SDK pulls in validmind-tracking-core automatically. It does not install the validmind package, aiohttp, or the full ML/testing dependency stack. Applications can pin either public package independently, for example validmind==2.13.11 or validmind-metrics==0.1.0.

Versioning and releases

The three distributions have independent versions and release tags:

Distribution Version source Release tag Build command
validmind Root pyproject.toml vX.Y.Z uv build
validmind-metrics packages/validmind-metrics/pyproject.toml metrics-vX.Y.Z uv build --package validmind-metrics
validmind-tracking-core packages/validmind-tracking-core/pyproject.toml tracking-core-vX.Y.Z uv build --package validmind-tracking-core

The existing validmind release process remains on vX.Y.Z. The metrics SDK is released and versioned separately on metrics-vX.Y.Z, so a metrics-only fix does not require a full-library release. The shared core is released separately when its API or transport changes; publish a compatible core version before publishing a dependent validmind or validmind-metrics release. The dependency ranges in each consumer package are updated when a new core major/minor compatibility line is required.

How to test

  • make lint
  • uv lock --check
  • uv build --all-packages --out-dir /tmp/validmind-library-build-3
  • uv run --package validmind-tracking-core python -m unittest discover packages/validmind-tracking-core/tests
  • uv run --package validmind-metrics python -m unittest discover packages/validmind-metrics/tests
  • uv pip check in a clean virtual environment after installing the built validmind-metrics wheel
  • Root unittest discovery reported 339 tests passing; the local process lingered during existing headless-browser/PostHog shutdown after the suite completed.

What needs special review?

  • OIDC device-flow, credential-cache, refresh-token, and bearer-token behavior in validmind-tracking-core.
  • The async boundary: metric requests use requests in a worker thread and do not share the full library's aiohttp session.
  • Independent package release ordering and the runtime dependency from validmind to validmind-tracking-core.

Dependencies, breaking changes, and deployment notes

  • Adds workspace distributions validmind-tracking-core and validmind-metrics.
  • The existing validmind wheel now depends on validmind-tracking-core; publish the core package before releasing a root-library version containing this dependency.
  • New release workflows use tracking-core-v*.*.* and metrics-v*.*.* tags.
  • No database migrations or new environment-variable names are introduced. Existing VM_API_* and VM_OIDC_* configuration is supported by the lightweight client.
  • Existing validmind.log_metric() and validmind.alog_metric() APIs remain available.

Release notes

Added a lightweight validmind-metrics SDK for sending ValidMind metrics from services without installing the full ML/testing stack. It supports API keys and OIDC authentication and is safe to use from async HTTP handlers.

Checklist

  • What and why
  • Screenshots or videos (Frontend) (N/A — no frontend changes)
  • How to test
  • What needs special review
  • Dependencies, breaking changes, and deployment notes
  • Labels applied
  • PR linked to Shortcut: sc-18115
  • Unit tests added (Backend)
  • Tested locally
  • Documentation updated (if required)
  • Environment variable additions/changes documented (if required)

@cachafla cachafla added enhancement New feature or request dependencies Pull requests that update a dependency file python Pull requests that update Python code security labels Aug 27, 2026
@cachafla
cachafla requested a review from nibalizer August 27, 2026 08:06
@cachafla
cachafla marked this pull request as ready for review August 27, 2026 08:17
@cachafla
cachafla requested a review from juanmleng August 28, 2026 16:35

@juanmleng juanmleng left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #556 at 67ce0c1 (merge-base 11ee7a2)

Verdict: Request changes (pending human confirmation)

Computed by the calibrated-review skill, run by @juanmleng.

Summary

  • What changed: Adds two new packages, validmind-tracking-core (auth + metric transport) and validmind-metrics (a thin public SDK), and moves the library's metric logging onto the shared transport via asyncio.to_thread.
  • Result: Two blocking findings. Both are about shipping the change and running it in a service, not about the async fix itself, which is correct.
  • Main thing to know: The next validmind release will depend on a package that is not on PyPI, so pip install validmind breaks until it is.
  • Next step: Add a release guard for the core package and a non-interactive mode for OIDC. The fix-now items are cheap and best done before the two new names are first published, because PyPI metadata can't be changed afterwards.

The async-boundary work is sound and the PR description held up: every factual claim I tested was true, including the environment-variable compatibility list.

Blocking findings

B1 · The next validmind release can publish a wheel nobody can install

  • Type: correctness
  • Direction: risky
  • What happens: validmind now hard-depends on validmind-tracking-core (>=0.1.0,<0.2.0), which does not exist on PyPI. The next run of the release workflow builds and publishes only the root package, so the published wheel names a dependency that can't be resolved.
  • Why it matters: Every pip install validmind fails until someone notices and publishes the core package. PyPI versions can't be changed after upload, so the fix is a yank plus a new version number, not a re-run. The required ordering is written down in three places (PR body, deployment notes, a comment in pypi.yaml) but nothing enforces it.
  • Evidence:
    • pyproject.toml:17 — new core runtime dependency on validmind-tracking-core
    • .github/workflows/pypi.yaml:41-42 — still a bare uv build / uv publish, which builds only the root package (the two workflows that need the core wheel had to add --all-packages)
    • curl https://pypi.org/pypi/validmind-tracking-core/json → 404 (re-checked 2026-10-02; validmind-metrics also 404)
    • git ls-remote --tags origin | grep tracking → no tracking-core-v* tag
  • Suggested fix: Before uv publish in pypi.yaml, add a step that resolves the required core version from the index and fails the job if it is missing. Alternatively, publish the core package from the same job first.

B2 · A metric call from an HTTP handler can block for 15 minutes waiting on an interactive login

  • Type: correctness
  • Direction: risky
  • What happens: With VM_OIDC_* set and no cached token, the first validmind_metrics.log_metric(...) in a process starts a device-authorization flow. It prints a URL and code to stdout, then polls for up to 900 seconds waiting for a human. A long-running server gets here on its own once a token expires without a usable refresh token.
  • Why it matters: In a request handler, the deployment this SDK is built for, that means a worker hung for 15 minutes, a login code nobody will ever see, and no metric. _get_client() has no lock, so concurrent first requests can each start their own flow.
  • Evidence:
    • packages/validmind-metrics/src/validmind_metrics/__init__.py:29-31 — _get_client() lazily calls init(), with no lock
    • packages/validmind-tracking-core/src/validmind_tracking_core/oidc.py:271 — initialize() falls through to run_device_flow (:302) when there is no valid cached entry
    • log_metric("accuracy", 0.9) with VM_OIDC_* set and an empty credentials dir → enters run_device_flow
    • status_callback replaces the printing but not the waiting, so there is no non-interactive mode
  • Suggested fix: Add interactive: bool = False to MetricsClient. When no usable credential exists, raise TrackingAuthError instead of prompting, and run device login only on explicit opt-in. Put a lock around _get_client().

Fix now

FN1

  • What: Concurrent token refreshes delete each other's cached credentials.
  • Why: upsert_cached_entry reads the file, modifies it and writes it back with no lock. The atomic write prevents a torn file but not a lost update. 25 threads each writing a distinct key left 4 entries. A multi-worker server, which this SDK targets, will hit this, and the symptom is auth that works and then suddenly asks you to log in again.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/credentials_store.py:115, :135
  • Suggested fix: Hold an exclusive fcntl.flock across the read-modify-write in both functions. A threading.Lock would not cover separate processes.

FN2

  • What: Both new packages declare AGPL but ship no licence text.
  • Why: Every source header points to a LICENSE file that no wheel or sdist contains (checked against full listings of all four artifacts). The root package handles this correctly. Adding the file is free before the first publish and needs a version bump after.
  • Where: packages/validmind-tracking-core/pyproject.toml:11, packages/validmind-metrics/pyproject.toml:11
  • Suggested fix: Put a LICENSE in each package directory and use license = { file = "LICENSE" }, as pyproject.toml:7 does.

FN3

  • What: Network failures escape the SDK's own error hierarchy.
  • Why: A caller who wraps logging in except TrackingError still gets requests.ConnectionError / Timeout raised at them, and those are the failures a service actually sees. oidc.py already wraps the same exceptions as TrackingAuthError.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:74
  • Suggested fix: Catch requests.RequestException around the post and re-raise it as a tracking error.

FN4

  • What: A blank VM_API_TIMEOUT stops any client from being constructed.
  • Why: VM_API_TIMEOUT= in a .env raises ValueError: could not convert string to float: '', and the message names neither the variable nor the SDK. Also, timeout=0 can't be set because 0 is falsy.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:123
  • Suggested fix: Treat a blank value as unset. Raise TrackingConfigurationError naming the variable when the value doesn't parse, and test timeout is not None.

FN5

  • What: True is accepted as a metric value and sent as JSON true.
  • Why: isinstance(True, int) is true, so log_metric("passed", True) posts a boolean to an endpoint that expects a number, with no error. Now that the check lives in the shared transport, this is the moment to tighten it.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:30
  • Suggested fix: Reject bool explicitly, and point the error message at the passed argument.

FN6

  • What: The same call works in the library and fails in the lightweight SDK when values are numpy types.
  • Why: params={"n": np.int64(5)} works through validmind (which passes NumpyEncoder) and raises TypeError: Object of type int64 is not JSON serializable through validmind-metrics. Separately, both SDKs reject np.int64 / np.float32 as the metric value.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:21
  • Suggested fix: Add a default encoder in tracking-core that calls .item() on any value that has it (this needs no numpy dependency). Let _validate_metric accept values that convert cleanly to int / float.

FN7

  • What: An invalid metric now logs "Error logging metric to ValidMind API" before raising.
  • Why: Validation moved inside the try in _send_metric_sync. A local typo is now reported as if it were a platform failure, and it can trigger a token refresh before failing.
  • Where: validmind/api_client.py:1014
  • Suggested fix: Call serialize_metric once, before the try.

FN8

  • What: On Python 3.9 and 3.10, token expiry is read as "already expired" for some valid timestamps.
  • Why: On Python before 3.11, datetime.fromisoformat rejects nanosecond precision and colon-less offsets. is_expired then reports the token as expired, so every metric call does a full refresh. Measured on 3.9.6 against 3.12.9. CI's default test run uses 3.9.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/credentials_store.py (is_expired)
  • Suggested fix: Before parsing, trim fractional seconds to six digits and insert the colon into the offset. Add a comment naming 3.11 as the reason.

FN9

  • What: Every token refresh fetches the OIDC discovery document again.
  • Why: That adds a round trip per refresh, and a slow or unavailable discovery endpoint stalls logging even while the token endpoint is healthy. The library's version cached this, and the copy dropped the cache. _token_endpoint also takes an entry argument it never uses.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/oidc.py:353-354
  • Suggested fix: Cache the discovery document per issuer, as validmind/oidc_device.py does, and remove the unused argument.

FN10

  • What: Nothing requires the identity provider to use HTTPS.
  • Why: An http:// issuer runs the whole device flow in the clear, including posting the refresh token. An internal IdP address typed without a scheme is an ordinary way to end up here.
  • Where: packages/validmind-tracking-core/src/validmind_tracking_core/oidc.py (normalize_issuer)
  • Suggested fix: Require https unless the host is loopback.

FN11

  • What: Package metadata is thin.
  • Why: On PyPI, neither package will show an author, project URLs or classifiers. validmind-tracking-core has a name close to the main package and almost nobody installs it directly, which is the profile a security scanner reads as a possible typosquat. Neither wheel contains py.typed, so mypy and pyright ignore all of the annotations.
  • Where: packages/validmind-tracking-core/pyproject.toml, packages/validmind-metrics/pyproject.toml
  • Suggested fix: Copy authors from the root manifest, add [project.urls], and put an empty py.typed beside each __init__.py.

FN12

  • What: The metrics README documents only half of the API.
  • Why: The README covers MetricsClient with explicit credentials. It leaves out the module-level init / log_metric / alog_metric and the environment variables the client reads, and the module-level path is the one that can start the interactive login in B2.
  • Where: packages/validmind-metrics/README.md
  • Suggested fix: Document the environment variables and the module-level functions, and say which auth mode suits a long-running service.

FN13

  • What: Smaller gaps in what this PR extends.
  • Why: These are cheap now, and the API ones get harder to change after the first publish.
  • Where:
    • scripts/verify_copyright.py:23 — checks only validmind/, though this PR extended lint and format to packages/
    • pyproject.toml:28 — new plotly <7 cap with no reason recorded
    • .github/workflows/dependency-testing.yaml — the new pip-freeze line runs only nightly or on manual dispatch, so it first executes after merge
    • MetricsClient.alog_metric(*args, **kwargs) — erases log_metric's typed signature
    • credentials_path_value vs credentials_path — two names for the same parameter
  • Suggested fix: Walk both roots in the copyright check. Drop the Plotly cap or comment on why it's there. Dispatch the pip-freeze job once against this branch. Mirror the signature in alog_metric. Pick one parameter name.

Fix later

FL1. The auth layer was copied into validmind-tracking-core instead of being shared. credentials_store.py and oidc.py are near-verbatim copies of validmind/credentials_store.py and validmind/oidc_device.py, and both copies own ~/.validmind/credentials.json. They have already diverged on how they choose the bearer token: the library matches login.microsoftonline.com as a substring (validmind/api_client.py:316), while the new package compares the parsed hostname. The same stored issuer can therefore select different tokens depending on which SDK reads it. This affects anyone running both SDKs against one cache. It doesn't block, because the two interoperate today and the metric transport, which is what the shared-transport goal is about, really is shared. Fix: have the library modules re-export from the core package and delete the duplicated code. Suggested owner: PR author.


FL2. When an OIDC token is refreshed off the event loop, the library's pooled aiohttp session can leak. Because metric sends now run in a worker thread, _invalidate_async_session (validmind/api_client.py:56-68) falls back to asyncio.run(sess.close()). That closes a session bound to the main loop from a new loop, and the error is swallowed. The cost is one leaked connection per refresh, for the life of a process that both logs metrics and uploads documentation. This is reasoned from the code, not reproduced. Fix: capture the loop when the session is created and close it with asyncio.run_coroutine_threadsafe. Suggested owner: library maintainers.


FL3. Both implementations decide which token Entra wants by matching a hard-coded login.microsoftonline.com. That excludes sovereign-cloud tenants (login.microsoftonline.us, login.partner.microsoftonline.cn) and ignores a configured audience. Fixing it needs a configuration surface, not an edit to the literal, so it's out of scope here. Suggested owner: whoever owns OIDC support.

Questions for a human

Q1

  • Owner: PR author, with product
  • Question: Is a customer blocked on this today, and by which part: the install size, or the event-loop errors in async handlers?
  • Context: The event-loop fix is self-contained. The library's alog_metric now hands a blocking request to a worker thread instead of creating an aiohttp session, and that change could ship in validmind with no new packages. The new packages are what bring in the release ordering, the two new PyPI projects and the separate tags. If the immediate need is the event-loop errors, shipping the fix first and the lightweight SDK once someone needs it would narrow this PR considerably. If a customer can't install the full stack in their service, the split is justified as it stands.

Q2

  • Owner: PR author
  • Question: Does validmind-tracking-core need to be its own published package, or could validmind depend on validmind-metrics directly?
  • Context: The core package exists so the full library and the lightweight SDK can share code, and its README says application code should not use it directly. It is still the package behind B1, and it adds a third PyPI project, tag and workflow. A two-package layout, with the full library built on the light one (the pattern MLflow uses with mlflow-skinny), would give the same sharing with one fewer release to sequence. If there's a reason the core must stay separate from the public SDK, such as a planned second consumer, it would help to say so in the PR body.

Q3

  • Owner: PR author
  • Question: Should the SDK send metrics on a small bounded background queue, or is a blocking call per metric the intended behaviour for the first release?
  • Context: alog_metric fixes the event-loop problem correctly, but each call holds a thread for a full round trip, with a 30 s default timeout. asyncio.to_thread uses the loop's default executor, which the host application shares. With its 12 local workers all busy, metric calls queue behind unrelated work. The synchronous log_metric called from a coroutine now blocks the loop for up to 30 s. Either choice is defensible, but the README's "safe to use from async HTTP handlers" reads as the stronger promise, so it should say which one this is.

Q4

  • Owner: PR author, with whoever consumes the header server-side
  • Question: What should X-LIBRARY-VERSION contain, and does anything on the platform parse it or gate on it?
  • Context: The header now takes three shapes. validmind_metrics.log_metric() sends 0.1.0, which is indistinguishable from an old full-library version. MetricsClient() sends validmind-tracking-core/0.1.0, even for a metrics user. The library sends 2.13.11. The defaults are in validmind_metrics/__init__.py:24 and metrics.py:107, and versions are hard-coded in five places across the two packages.

Q5

  • Owner: whoever holds the PyPI token
  • Question: Is the PyPI token used for publishing account-scoped or project-scoped?
  • Context: A token scoped to the validmind project cannot create the two new PyPI projects, so both new publish workflows would fail on their first run. This is independent of B1.

Q6

  • Owner: whoever owns bearer-token validation on the platform
  • Question: Does the platform reject an id_token from an issuer it doesn't recognise, or accept it as an identity?
  • Context: This decides whether the library's substring match on the issuer host is only hygiene or a real authentication weakness. The selected token is sent only to the ValidMind API, never to the issuer, so a misclassification today shows up as a 401, not a token leak.

Accepted risks

  • What I checked: The CodeQL alert on oidc.py ("incomplete URL substring sanitization").
  • Why it is acceptable: It was raised on the first commit. The next commit ("Harden OIDC issuer matching") replaced the substring check in the new package with a parsed-hostname comparison. The substring form remains only in the library and is covered by FL1, FL3 and Q4.

  • What I checked: Both sdists include .gitignore even though the include list doesn't name it.
  • Why it is acceptable: Hatchling adds it on its own, and it is harmless. It's worth knowing only as a sign that the include list isn't what decides sdist contents.

  • What I checked: _get_url modifies the module-level _api_host from a worker thread.
  • Why it is acceptable: The change is idempotent (it appends a trailing slash once), so a race produces the same value.

Verification performed

All runs were done in a detached worktree at the head commit, which hasn't changed since they were run.

  • pytest on both new package suites → 2/2 and 4/4 passed
  • uv build --all-packages → four artifacts built
    • full file listings and METADATA extracted, which is the source of the licence, py.typed and metadata items
  • 25-thread upsert_cached_entry on distinct keys → 4 of 25 entries survived
  • validmind_metrics.log_metric with VM_OIDC_* set and an empty credentials dir → entered run_device_flow
  • serialize_metric / post_metric across every validation and error branch → ConnectionError / Timeout escape, and True accepted
  • params={"n": np.int64(5)} through both SDKs → works in the library, TypeError in validmind-metrics
  • VM_API_TIMEOUT="" → ValueError at construction
  • X-LIBRARY-VERSION captured per entry point → 0.1.0, validmind-tracking-core/0.1.0, 2.13.11
  • Expiry parsing on CPython 3.9.6 vs 3.12.9 → nanosecond and +0000 forms are treated as expired on 3.9 only
  • asyncio.to_thread executor identity and saturation → same default executor as run_in_executor(None, …), 12-worker ceiling locally, and queued calls wait
  • Bearer-token selection across seven issuer forms → the library and the new package disagree on lookalike hosts
  • curl https://pypi.org/pypi/<name>/json for both new names → 404 (re-checked 2026-10-02)
  • git log <merge-base>..origin/main → 34 commits. None touch the workflows, the credential store, api_client.py or packages/
  • CI on the head → 26/26 completed checks green
  • Session leak when a token is refreshed off the loop → NOT EXECUTED, reasoned from the code
  • PR claim "no new environment variables" → checked, true
  • PR claim of compatibility with the existing environment variables → checked, true
  • Full library test suite → NOT RUN
  • Behaviour against a live ValidMind API or a real identity provider → NOT CHECKED

- Fail the validmind release if the required validmind-tracking-core
  version is not on PyPI
- Make OIDC non-interactive by default (interactive=True opts into device
  login) and lock lazy client creation
- Lock credential-file read-modify-write; parse expiry on Python < 3.11
- Wrap network errors as TrackingConnectionError; export TrackingError
- Reject bool metric values; accept numpy-style scalars without numpy
- Treat blank VM_API_TIMEOUT as unset and allow timeout=0
- Validate metrics before the send path in the library
- Cache OIDC discovery per authenticator; require https issuers
- Ship LICENSE, py.typed, authors and URLs in both packages
- Document module-level API, env vars, auth and async behaviour
- Extend copyright check to packages/; note why Plotly is capped

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cachafla

cachafla commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@juanmleng thanks for the thorough review. Everything in Blocking and Fix now is addressed in afc3361. Checks: package suites pass on Python 3.11 and 3.9 (17 tests, 13 new in test_review_fixes.py), tests.test_api_client + tests.test_results pass (72), make lint / format / verify_copyright are clean, and uv build --all-packages artifacts now contain LICENSE and py.typed.

Blocking

  • B1 pypi.yaml now has a "Check validmind-tracking-core is on PyPI" step before publishing. It reads the tracking-core requirement from pyproject.toml and runs pip download --no-deps against it, so the job fails if that version isn't published yet. I tested both cases locally: the step passes for a published spec and fails with "No matching distribution" for validmind-tracking-core today.
  • B2 OIDCAuthenticator / MetricsClient take interactive: bool = False. With no usable cached or refreshable token they raise TrackingAuthError, and the device flow only runs with interactive=True. validmind_metrics._get_client() / init() now hold a lock. Test: test_non_interactive_by_default.

Fix now

  • FN1 Upsert and delete hold an exclusive fcntl.flock on a sidecar credentials.json.lock for the whole read-modify-write. A sidecar is needed because the data file is replaced atomically. The 25-thread repro now keeps 25/25 entries (test_concurrent_upserts_keep_every_entry). Windows has no fcntl, so it falls back to no cross-process lock; that limit is marked in a code comment.
  • FN2 LICENSE was added to each package with license = { file = "LICENSE" } and included in the sdist. Verified in all four artifacts.
  • FN3 post_metric wraps requests.RequestException as the new TrackingConnectionError(TrackingError). TrackingError is now exported from both packages.
  • FN4 Added timeout_from_env(): a blank value counts as unset, and a value that doesn't parse raises TrackingConfigurationError naming VM_API_TIMEOUT. The client now tests timeout is not None, so timeout=0 is honoured. The library's metric path uses the same helper.
  • FN5 bool values are rejected with a message pointing at passed. This also applies to validmind.log_metric, since it goes through serialize_metric.
  • FN6 The default encoder is now ScalarEncoder (.tolist(), no numpy import), and the metric value is unwrapped with .item() when it isn't already an int or float. np.int64 / np.float32 values and numpy params now work in both SDKs.
  • FN7 The library serializes and validates the metric in log_metric / alog_metric before the send path. A bad argument raises ValueError without the "Error logging metric to ValidMind API" log line and without a token refresh. Real send failures, API errors included, still log.
  • FN8 Added _parse_timestamp, which normalizes Z, fractional seconds of any length (to 6 digits) and +HHMM offsets before fromisoformat. The comment names Python 3.11 as the reason. Tested on 3.9.
  • FN9 The discovery document is cached on the authenticator, which is per issuer, and passed into run_device_flow. The unused entry argument to _token_endpoint is gone (test_discovery_fetched_once).
  • FN10 OIDCAuthenticator requires https. http is allowed only for localhost / 127.0.0.1 / ::1, and a scheme-less issuer is rejected.
  • FN11 Both packages now have authors (copied from the root), [project.urls], classifiers including Typing :: Typed, and an empty py.typed.
  • FN12 The README now covers the module-level init / log_metric / alog_metric, the env-var table, which auth suits services (API key, with OIDC non-interactive after a one-time init(interactive=True)), async semantics and the error hierarchy.
  • FN13
    • verify_copyright.py now walks validmind/ and packages/. The package headers were updated to match scripts/copyright.txt.
    • The Plotly cap is kept, with a comment explaining it. Plotly 7.1.0 is out, and the cap holds us on 6.x until the figure code and kaleido export are validated against it.
    • I dispatched dependency-testing.yaml on this branch so the pip-freeze leg runs before merge: https://github.com/validmind/validmind-library/actions/runs/37022543817
    • MetricsClient.alog_metric now mirrors log_metric's signature.
    • credentials_path_value was renamed to credentials_path.

Fix later: no code changes in this PR.

  • FL1 Agreed. The library modules should re-export from tracking-core, and the duplicated credentials_store.py / oidc_device.py should be deleted. That also removes the substring match. It needs a follow-up story.
  • FL2 Agreed that it is reasoned but not reproduced. The fix is to capture the session's loop at creation and close it with run_coroutine_threadsafe. I'll leave that to the library maintainers.
  • FL3 Agreed. Sovereign-cloud Entra needs a configuration surface and is out of scope here.

Questions (cc @cachafla for Q1/Q2/Q4)

  • Q1 / Q2 Product questions about scope and package layout. These are for the PR author to answer.
  • Q3 A blocking call per metric is the first-release behaviour. The README no longer says "safe to use from async handlers". It now says each alog_metric holds one default-executor thread for a full round trip with no background queue, and that sync log_metric from a coroutine blocks the loop.
  • Q4 I left the header values unchanged because I don't know whether the platform parses X-LIBRARY-VERSION. If it doesn't, I'd suggest sending validmind-metrics/<ver> from the metrics SDK so it can't be confused with a library version.
  • Q5 These need someone with PyPI and platform access: Q5 is the token's scope, which needs PyPI admin access to check.
  • Q6 Platform id_token issuer validation. This needs someone on the platform side who owns bearer-token validation.

@juanmleng

Copy link
Copy Markdown
Contributor

Thanks @cachafla! I went through afc3361 against the review and everything holds up: the release guard, the non-interactive default (my original repro now raises TrackingAuthError without starting the device flow), the credentials lock, the numpy and bool handling, the licence and py.typed in all four artifacts, and the README env-var table matches exactly what the code reads.

One small thing left: the pip-freeze leg still hasn't run. The dispatched run used the default test_type=all, and that job only runs when test_type=pip-freeze, so it was skipped in both runs. Could you kick it off once more with gh workflow run dependency-testing.yaml --ref cachafla/sc-18115/lightweight-metrics-sdk -f test_type=pip-freeze?

Also worth a line in the release notes: validmind.log_metric("x", True) used to send true and now raises ValueError. That's the change I asked for, but it's visible to existing library users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request python Pull requests that update Python code security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants