Repository navigation
Conversation
There was a problem hiding this comment.
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) andvalidmind-metrics(a thin public SDK), and moves the library's metric logging onto the shared transport viaasyncio.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
validmindrelease will depend on a package that is not on PyPI, sopip install validmindbreaks 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:
validmindnow hard-depends onvalidmind-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 validmindfails 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 inpypi.yaml) but nothing enforces it. - Evidence:
pyproject.toml:17— new core runtime dependency onvalidmind-tracking-core.github/workflows/pypi.yaml:41-42— still a bareuv 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-metricsalso 404)git ls-remote --tags origin | grep tracking→ notracking-core-v*tag
- Suggested fix: Before
uv publishinpypi.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 firstvalidmind_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 callsinit(), with no lockpackages/validmind-tracking-core/src/validmind_tracking_core/oidc.py:271—initialize()falls through torun_device_flow(:302) when there is no valid cached entrylog_metric("accuracy", 0.9)withVM_OIDC_*set and an empty credentials dir → entersrun_device_flowstatus_callbackreplaces the printing but not the waiting, so there is no non-interactive mode
- Suggested fix: Add
interactive: bool = FalsetoMetricsClient. When no usable credential exists, raiseTrackingAuthErrorinstead 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_entryreads 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.flockacross the read-modify-write in both functions. Athreading.Lockwould 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
LICENSEin each package directory and uselicense = { file = "LICENSE" }, aspyproject.toml:7does.
FN3
- What: Network failures escape the SDK's own error hierarchy.
- Why: A caller who wraps logging in
except TrackingErrorstill getsrequests.ConnectionError/Timeoutraised at them, and those are the failures a service actually sees.oidc.pyalready wraps the same exceptions asTrackingAuthError. - Where:
packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:74 - Suggested fix: Catch
requests.RequestExceptionaround the post and re-raise it as a tracking error.
FN4
- What: A blank
VM_API_TIMEOUTstops any client from being constructed. - Why:
VM_API_TIMEOUT=in a.envraisesValueError: could not convert string to float: '', and the message names neither the variable nor the SDK. Also,timeout=0can't be set because0is falsy. - Where:
packages/validmind-tracking-core/src/validmind_tracking_core/metrics.py:123 - Suggested fix: Treat a blank value as unset. Raise
TrackingConfigurationErrornaming the variable when the value doesn't parse, and testtimeout is not None.
FN5
- What:
Trueis accepted as a metric value and sent as JSONtrue. - Why:
isinstance(True, int)is true, solog_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
boolexplicitly, and point the error message at thepassedargument.
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 throughvalidmind(which passesNumpyEncoder) and raisesTypeError: Object of type int64 is not JSON serializablethroughvalidmind-metrics. Separately, both SDKs rejectnp.int64/np.float32as 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_metricaccept values that convert cleanly toint/float.
FN7
- What: An invalid metric now logs "Error logging metric to ValidMind API" before raising.
- Why: Validation moved inside the
tryin_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_metriconce, before thetry.
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.fromisoformatrejects nanosecond precision and colon-less offsets.is_expiredthen 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_endpointalso takes anentryargument 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.pydoes, 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
httpsunless 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-corehas 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 containspy.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
authorsfrom the root manifest, add[project.urls], and put an emptypy.typedbeside each__init__.py.
FN12
- What: The metrics README documents only half of the API.
- Why: The README covers
MetricsClientwith explicit credentials. It leaves out the module-levelinit/log_metric/alog_metricand 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 onlyvalidmind/, though this PR extended lint and format topackages/pyproject.toml:28— newplotly <7cap 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 mergeMetricsClient.alog_metric(*args, **kwargs)— eraseslog_metric's typed signaturecredentials_path_valuevscredentials_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_metricnow hands a blocking request to a worker thread instead of creating anaiohttpsession, and that change could ship invalidmindwith 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-coreneed to be its own published package, or couldvalidminddepend onvalidmind-metricsdirectly? - 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_metricfixes the event-loop problem correctly, but each call holds a thread for a full round trip, with a 30 s default timeout.asyncio.to_threaduses the loop's default executor, which the host application shares. With its 12 local workers all busy, metric calls queue behind unrelated work. The synchronouslog_metriccalled 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-VERSIONcontain, and does anything on the platform parse it or gate on it? - Context: The header now takes three shapes.
validmind_metrics.log_metric()sends0.1.0, which is indistinguishable from an old full-library version.MetricsClient()sendsvalidmind-tracking-core/0.1.0, even for a metrics user. The library sends2.13.11. The defaults are invalidmind_metrics/__init__.py:24andmetrics.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
validmindproject 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_tokenfrom 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
.gitignoreeven 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_urlmodifies the module-level_api_hostfrom 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.
pyteston both new package suites → 2/2 and 4/4 passeduv build --all-packages→ four artifacts built- full file listings and
METADATAextracted, which is the source of the licence,py.typedand metadata items
- full file listings and
- 25-thread
upsert_cached_entryon distinct keys → 4 of 25 entries survived validmind_metrics.log_metricwithVM_OIDC_*set and an empty credentials dir → enteredrun_device_flowserialize_metric/post_metricacross every validation and error branch →ConnectionError/Timeoutescape, andTrueacceptedparams={"n": np.int64(5)}through both SDKs → works in the library,TypeErrorinvalidmind-metricsVM_API_TIMEOUT=""→ValueErrorat constructionX-LIBRARY-VERSIONcaptured 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
+0000forms are treated as expired on 3.9 only asyncio.to_threadexecutor identity and saturation → same default executor asrun_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>/jsonfor 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.pyorpackages/- 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>
|
@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 Blocking
Fix now
Fix later: no code changes in this PR.
Questions (cc @cachafla for Q1/Q2/Q4)
|
|
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 One small thing left: the pip-freeze leg still hasn't run. The dispatched run used the default Also worth a line in the release notes: |
What and why?
Adds a dependency-light
validmind-metricsSDK and sharedvalidmind-tracking-corepackage 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 anaiohttpsession. 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:
Users who only need to send ValidMind metrics can install the lightweight SDK:
The metrics SDK pulls in
validmind-tracking-coreautomatically. It does not install thevalidmindpackage,aiohttp, or the full ML/testing dependency stack. Applications can pin either public package independently, for examplevalidmind==2.13.11orvalidmind-metrics==0.1.0.Versioning and releases
The three distributions have independent versions and release tags:
validmindpyproject.tomlvX.Y.Zuv buildvalidmind-metricspackages/validmind-metrics/pyproject.tomlmetrics-vX.Y.Zuv build --package validmind-metricsvalidmind-tracking-corepackages/validmind-tracking-core/pyproject.tomltracking-core-vX.Y.Zuv build --package validmind-tracking-coreThe existing
validmindrelease process remains onvX.Y.Z. The metrics SDK is released and versioned separately onmetrics-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 dependentvalidmindorvalidmind-metricsrelease. The dependency ranges in each consumer package are updated when a new core major/minor compatibility line is required.How to test
make lintuv lock --checkuv build --all-packages --out-dir /tmp/validmind-library-build-3uv run --package validmind-tracking-core python -m unittest discover packages/validmind-tracking-core/testsuv run --package validmind-metrics python -m unittest discover packages/validmind-metrics/testsuv pip checkin a clean virtual environment after installing the builtvalidmind-metricswheelWhat needs special review?
validmind-tracking-core.requestsin a worker thread and do not share the full library'saiohttpsession.validmindtovalidmind-tracking-core.Dependencies, breaking changes, and deployment notes
validmind-tracking-coreandvalidmind-metrics.validmindwheel now depends onvalidmind-tracking-core; publish the core package before releasing a root-library version containing this dependency.tracking-core-v*.*.*andmetrics-v*.*.*tags.VM_API_*andVM_OIDC_*configuration is supported by the lightweight client.validmind.log_metric()andvalidmind.alog_metric()APIs remain available.Release notes
Added a lightweight
validmind-metricsSDK 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