Skip to content

fix(models): Prefix digit-leading upstream model ids#65

Merged
matthewgrossman merged 1 commit into
mainfrom
fix-yi-large-logs/mgrossman
May 27, 2026
Merged

fix(models): Prefix digit-leading upstream model ids#65
matthewgrossman merged 1 commit into
mainfrom
fix-yi-large-logs/mgrossman

Conversation

@matthewgrossman
Copy link
Copy Markdown
Contributor

@matthewgrossman matthewgrossman commented May 27, 2026

What

When the models controller reconciler discovers models from an external provider, it normalizes each upstream id to an internal entity name that satisfies the entity store's RFC 1035-style NAME_PATTERN (leading [a-z], length 2-63, only [a-z0-9-]). Upstream catalogs occasionally publish digit-leading ids — e.g. NVIDIA Build's 01-ai/yi-large — which the normalizer rejects because 01-ai-yi-large doesn't start with a letter. The reconciler then logs Skipped 1 model(s) with invalid names ... on every cycle (~every 5s) and the model never becomes routable.

This PR teaches normalize_model_entity_name to prepend an internal m- prefix when the cleaned form starts with a digit, so digit-leading upstream ids become valid entity names instead of being skipped.

Why this approach

  • Internal-only, not user-facing. The normalized string is the entity store's model_entity_id. The served_model_name mapping still preserves the original id (provider_reconciler.py:707), so the IGW continues to forward 01-ai/yi-large (the original) to the backend. End users keep calling the model exactly as the upstream advertises it.
  • Minimal & deterministic. A constant m- prefix is mechanical, idempotent (m-01-ai-yi-largem-01-ai-yi-large), and preserves the source id verbatim after the prefix, so the entity name still reads like the upstream id.
  • Length-safe. The prefix is applied before the existing 63-char truncate-with-hash step, so long digit-leading names still produce length-bounded results via the existing machinery.

Considered and rejected: provider-derived prefixes (collide with the existing workspace/name convention, lengthen names), spelling out leading digits (lossy/weird), dropping leading digits (collisions across ids that differ only in the digit segment), hash prefixes (ugly, inconsistent with the existing policy of hashing only on overflow).

Verification

  • New positive test cases for 01-ai/yi-large, 9model, 123, 2pac/rapper-model.
  • New dedicated tests for idempotence and the digit-leading + 63-char-overflow interaction.
  • Updated test_entity_name_from_discovered_model_invalid_raises and added test_entity_name_from_discovered_model_digit_leading_gets_prefix so the reconciler's caller-side contract is also pinned.
  • Full services/core/models/tests/unit suite: 992 passed, 1 skipped.
  • ruff check / ruff format --check clean. ty clean for the change (5 pre-existing diagnostics in unrelated functions in the same file).

Tracking

Signed-off-by: Matthew Grossman mgrossman@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved normalization of model identifiers that start with digits to ensure proper system compatibility.
    • Updated validation logic to handle digit-leading model IDs with automatic prefix handling.
  • Tests

    • Added test coverage for digit-leading model identifier normalization and edge cases.

Review Change Stack

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
@matthewgrossman matthewgrossman requested review from a team as code owners May 27, 2026 00:24
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

📝 Walkthrough

Walkthrough

The PR updates model identifier normalization to handle digit-leading IDs by prefixing them with m- before truncation. Implementation, unit tests, and integration tests all reflect this new behavior; digit-leading identifiers are no longer treated as invalid but normalized with the internal prefix while preserving the original as a user-facing handle.

Changes

Digit-leading Normalization with m- Prefix

Layer / File(s) Summary
Normalization logic and documentation
services/core/models/src/nmp/core/models/app/utils.py
normalize_model_entity_name adds m- prefix when normalized output starts with a digit, applied before truncation/validation. Docstring updated with new behavior description and digit-leading example ("01-ai/yi-large""m-01-ai-yi-large").
Unit tests for digit-leading normalization
services/core/models/tests/unit/common/test_utils.py
Parametrized test matrix extended with digit-leading inputs now expecting m- prefix in normalized output. Invalid-input test matrix adjusted to remove digit-leading cases. New test cases verify idempotency and correct truncation behavior for long digit-leading identifiers with hash suffix.
Integration tests for discovered models
services/core/models/tests/unit/controllers/test_provider_reconciler.py
Invalid-entity-name test narrowed to single-character ID case. New test added verifying digit-leading discovered model IDs (e.g., "01-ai/yi-large", "123") receive m- prefix in resulting entity names.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title directly describes the main change: handling digit-leading upstream model IDs by adding m- prefix, which is the core fix across documentation, implementation, and tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-yi-large-logs/mgrossman

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

Suite Lines Covered Line Rate Branch Rate
Unit Tests 18245/24195 75.4% 61.8%
Integration Tests 11665/22977 50.8% 25.9%

@mckornfield
Copy link
Copy Markdown
Contributor

lmao jinx #67

Copy link
Copy Markdown
Contributor

@mckornfield mckornfield left a comment

Choose a reason for hiding this comment

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

m- is probably better than md- and mine has lint failures

@matthewgrossman matthewgrossman added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit 1c8c81c May 27, 2026
13 checks passed
aray12 pushed a commit that referenced this pull request May 28, 2026
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Alex Ray <alray@nvidia.com>
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.

2 participants