Skip to content

Reject nested metric names that conflict with scalar parents - #909

Open
kshitijprasaddev wants to merge 2 commits into
treeverse:mainfrom
kshitijprasaddev:fix/nested-metric-parent-conflict
Open

kshitijprasaddev wants to merge 2 commits into
treeverse:mainfrom
kshitijprasaddev:fix/nested-metric-parent-conflict

Conversation

@kshitijprasaddev

@kshitijprasaddev kshitijprasaddev commented Oct 27, 2026 •

Copy link
Copy Markdown

Logging a scalar metric and then a nested metric beneath it raises an internal AttributeError after the nested TSV has already been written. For example, log_metric("epoch", 0) followed by log_metric("epoch/loss", 0.5) leaves history that is absent from the summary.

Validate each existing summary parent before registering or dumping the metric. Raise InvalidMetricNameError naming the conflict. This also checks resumed summaries and the internal step value; normal sibling metrics and repeated leaf updates still work. The log_metric docstring documents the error.

Fixes #492.

Validation (Ubuntu 24.04, CPython 3.12.3):

  • The same 15 added cases produce 14 failures and one control pass with the original implementation; all 15 pass with the fix. They cover integer-zero/float/string parents, nested prefixes, plot=False, resume, internal step, and successful sibling/leaf updates. Rejections preserve summary, registry and history files.
  • Complete core suite (excluding optional framework integrations): 246 passed, compared with 231 passed on a pristine baseline. Both emit the same 68 existing warnings (DVC/pathspec deprecations and existing tests passing Path objects to setenv).
  • Pre-commit on all changed files and whole-project mypy (57 files) pass. The all-files pre-commit run passes every applicable hook except existing pyproject.toml inline-table spacing; its two unrelated formatting changes were reverted.
  • Isolated source/wheel builds and twine check pass. Whitespace checks pass.

Optional ML framework integrations, other Python versions/platforms, and hosted CI are not claimed as tested. An initial coverage-enabled core run stalled in the asynchronous Studio tests and was interrupted; the subsequent complete coverage-enabled run passed all 246 cases. No production coverage percentage is claimed.

The current scope detects scalar parents when logging descendants; it does not change the existing reverse-order behavior of overwriting a subtree with a scalar.

AI assistance: OpenAI Codex investigated the issue, implemented and reviewed the patch, ran the reported validation, and prepared this PR. Human review is not claimed.

  • I followed the contribution guide, adding regression tests and documenting the error. Existing baseline warnings and formatting limitation are reported above.
  • No separate dvc.org documentation update is needed for this error-handling fix; the API docstring is updated.

Follow-up — 2026-10-28

pre-commit.ci added commit 93c7a2a with the two existing TOML inline-table spacing fixes. The parsed configuration is unchanged. The branch has been synced locally and the complete all-files pre-commit run now passes. The test results above refer to the unchanged production/test source from 810baee; no new runtime changes were made in this follow-up.

The CLA assistant now reports an unsigned Contributor License Agreement. Signing requires the account holder; no agreement or signature has been supplied by the agent.

@github-project-automation github-project-automation Bot moved this to Backlog in DVC Oct 27, 2026
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

log_metric: raise helpful error if a nested metric is logged and parent key exists as a metric

2 participants