fix: break models.errors import cycle by Thanatos9404 · Pull Request #1039 · NVIDIA-NeMo/DataDesigner

Thanatos9404 · 2026-10-15T20:08:57Z

📋 Summary

Importing data_designer.engine.models.errors or data_designer.engine.models.registry as the first Data Designer import in a fresh process raises ImportError, because clients/factory.py imports FormattedLLMErrorMessage back from models/errors.py while errors.py is still loading the clients package. This moves FormattedLLMErrorMessage into engine/models/utils.py, following the approach in the issue, so that back edge goes away and every import stays at module level.

🔗 Related Issue

Fixes #990

🔄 Changes

Move FormattedLLMErrorMessage from engine/models/errors.py to engine/models/utils.py, which does not import the clients package. Fields and __str__ are unchanged; the only addition is a one-line docstring.
Import it from engine/models/utils.py in both engine/models/clients/factory.py and engine/models/errors.py. errors.py keeps the name, so from data_designer.engine.models.errors import FormattedLLMErrorMessage still works.
New tests/engine/models/test_import_order.py: fresh-subprocess imports of models.errors and models.registry, a check that both import paths give the same class, and a check that the clients package still exports create_model_client.
test_unknown_provider_type_raises_data_designer_error now pins the exact error type (DataDesignerError) and the full formatted message.

🧪 Testing

Before the fix (main at 7760799 with only the new tests applied):

uv run --group dev pytest \
  packages/data-designer-engine/tests/engine/models/test_import_order.py \
  packages/data-designer-engine/tests/engine/models/clients/test_factory.py::test_unknown_provider_type_raises_data_designer_error \
  packages/data-designer/tests/test_lazy_imports.py::test_health_checks_entrypoint_imports_in_fresh_process
3 failed, 3 passed

Both fresh-process imports fail with the ImportError from the issue, and the identity check fails because utils has no FormattedLLMErrorMessage yet. The export, unsupported-provider and health-check tests pass before and after, as guards.

With the fix, the same command gives 6 passed.

Importing every module under packages/*/src as the first import in its own fresh process: 2 of 312 fail on main (models.errors and models.registry), 0 of 312 on this branch.

No inline imports were added, create_model_client is still exported from data_designer.engine.models.clients, and the health-check import regression in packages/data-designer/tests/test_lazy_imports.py is unchanged and still passes. import data_designer.config plus from data_designer.interface import DataDesigner loads the same 1783 modules on main and on this branch.

make check-all and make test pass locally, apart from 8 tests that fail the same way on main in my environment (7 need jq, test_import_perf.py timed out).

make test passes (apart from the 8 environment-only failures above, which also fail on main locally)
Unit tests added/updated
E2E tests added/updated (if applicable): not applicable, import-order fix only

✅ Checklist

Follows commit message conventions
Commits are signed off (DCO)
If I am an external contributor, the linked issue is open and labeled triaged
Architecture docs updated (if applicable): not applicable, no architecture or Fern page mentions this class

Importing data_designer.engine.models.errors or .registry as the first Data Designer import in a process raised ImportError: errors.py loads the clients package, whose factory imported FormattedLLMErrorMessage back from the still partially initialized errors.py. Move FormattedLLMErrorMessage into engine/models/utils.py, which does not import the clients package, and import it from there in both clients/factory.py and models/errors.py. errors.py still exposes the name, so existing imports keep working. Fields and formatting are unchanged. Add fresh-process import tests for models.errors and models.registry, check that both import paths give the same class and that the clients package still exports create_model_client, and pin the exact type and text of the unsupported-provider error. Fixes NVIDIA-NeMo#990 Signed-off-by: Yashvardhan Thanvi <[email protected]>

github-actions · 2026-10-15T20:09:08Z

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

Thanatos9404 · 2026-10-15T20:11:26Z

I have read the DCO document and I hereby sign the DCO.

greptile-apps · 2026-10-15T20:11:51Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Moves FormattedLLMErrorMessage into models/utils.py to break the import cycle.

Fresh processes can import model errors and registries first.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  E["models.errors"] --> C["clients package"]
  C --> F["clients.factory"]
  F --> U["models.utils"]
  E --> U
  U --> M["FormattedLLMErrorMessage"]

_{Reviews (2) · Last reviewed commit: "Merge branch 'main' into Thanatos9404/fi..." · Reviewed by Greptile}

nabinchha · 2026-10-15T20:36:41Z

Thanks for putting this together, @Thanatos9404!

Summary

Moving FormattedLLMErrorMessage into models.utils removes the import-cycle back edge while preserving its existing import path and formatting behavior. The implementation matches the PR description. Reviewed commit 7bbe04be919a124117761ef57a40ae53896aa832.

Findings

No actionable findings.

What Looks Good

The fix removes the dependency causing the cycle without changing client exports or error-handling behavior.
Fresh-process tests exercise both failing import entry points; compatibility checks preserve class identity across the old and new paths.
The unsupported-provider test checks the exact exception type and formatted message.

Residual Risk

Validation covered 122 targeted tests across import order, client factory, model errors, utilities, registry, and lazy imports; all passed. Ruff lint and formatting checks passed for all five changed files. An additional ten-pass review exercised all 741 model tests: 739 passed in the sandbox, and the two tests blocked by loopback socket permissions passed on rerun with permission. All 41 model modules imported first in fresh processes, all 24 tested import-order permutations passed, and formatter equivalence and legacy pickle checks passed. The full repository suite was not run. The PR head was reconfirmed before posting.

A focused security review found no issues introduced by this patch: no dependency or workflow changes, new credential access, outbound requests, or shell execution.

Verdict

Ship it — no changes requested.

This review was generated by an AI assistant.

nabinchha

Approved!

Thanatos9404 requested a review from a team as a code owner October 15, 2026 20:08

Merge branch 'main' into Thanatos9404/fix/990-models-errors-import-cycle

48d7569

nabinchha approved these changes Oct 15, 2026

View reviewed changes

nabinchha merged commit 86547c6 into NVIDIA-NeMo:main Oct 15, 2026
67 checks passed

Navigation Menu

Repository navigation

fix: break models.errors import cycle - #1039

fix: break models.errors import cycle#1039
nabinchha merged 2 commits into
NVIDIA-NeMo:mainfrom
Thanatos9404:Thanatos9404/fix/990-models-errors-import-cycle

Thanatos9404 commented Oct 15, 2026

Uh oh!

github-actions Bot commented Oct 15, 2026 •

edited

Loading

Uh oh!

Thanatos9404 commented Oct 15, 2026

Uh oh!

greptile-apps Bot commented Oct 15, 2026 •

edited

Loading

Diagram

Uh oh!

nabinchha commented Oct 15, 2026

Uh oh!

nabinchha left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Thanatos9404 commented Oct 15, 2026

📋 Summary

🔗 Related Issue

🔄 Changes

🧪 Testing

✅ Checklist

Uh oh!

github-actions Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thanatos9404 commented Oct 15, 2026

Uh oh!

greptile-apps Bot commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Diagram

Uh oh!

nabinchha commented Oct 15, 2026

Summary

Findings

What Looks Good

Residual Risk

Verdict

Uh oh!

nabinchha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 15, 2026 •

edited

Loading

greptile-apps Bot commented Oct 15, 2026 •

edited

Loading

nabinchha left a comment •

edited

Loading