Skip to content

fix: update health checks to use new ModelFacade client API#470

Merged
andreatgretel merged 2 commits intomainfrom
andreatgretel/fix/health-checks-facade-api
Mar 30, 2026
Merged

fix: update health checks to use new ModelFacade client API#470
andreatgretel merged 2 commits intomainfrom
andreatgretel/fix/health-checks-facade-api

Conversation

@andreatgretel
Copy link
Copy Markdown
Contributor

📋 Summary

The ModelFacade constructor was refactored in #373 to require a ModelClient instead of a SecretResolver, but scripts/health_checks.py was never updated. This causes a TypeError (exit code 2) on every Health Checks workflow run since March 9.

🐛 Fixed

  • Updated scripts/health_checks.py to use create_model_client() factory and pass the resulting client to ModelFacade via the client keyword argument

🤖 Generated with AI

The ModelFacade constructor was refactored in #373 to accept a
ModelClient instead of a SecretResolver. The health_checks.py script
was not updated, causing a TypeError (exit code 2) on every run
since March 9.
@andreatgretel andreatgretel requested a review from a team as a code owner March 30, 2026 12:33
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR is a targeted one-line bugfix that brings scripts/health_checks.py up to date with the ModelFacade API refactor from #373. The old code passed a SecretResolver directly to ModelFacade, which no longer accepts that argument and caused a TypeError on every CI health-check run. The fix correctly introduces an intermediate call to create_model_client() to produce a ModelClient, then passes it via the client= keyword argument to ModelFacade.

  • The argument order for create_model_client(model_config, secret_resolver, provider_registry) matches the factory's positional signature exactly.
  • The ModelFacade(model_config, provider_registry, client=client) call matches the constructor's required keyword-only client parameter.
  • No other callers in the repository appear to be affected by this change; the scope is limited to this one script.

Confidence Score: 5/5

  • Safe to merge — the fix correctly aligns the health check script with the post-refactor: Decouple ModelFacade from LiteLLM via ModelClient adapter #373 ModelFacade API.
  • The change is minimal (two lines), targets a confirmed TypeError, and the new call signatures match both create_model_client and ModelFacade.__init__ exactly. No logic changes, no new dependencies, and no other files are affected.
  • No files require special attention.

Important Files Changed

Filename Overview
scripts/health_checks.py Updated _check_model to use create_model_client() factory + pass client= kwarg to ModelFacade, matching the new API introduced in #373.

Sequence Diagram

sequenceDiagram
    participant HC as health_checks.py
    participant F as create_model_client()
    participant SR as EnvironmentResolver
    participant PR as ModelProviderRegistry
    participant C as ModelClient
    participant MF as ModelFacade

    HC->>SR: EnvironmentResolver()
    HC->>PR: _get_provider_registry(provider_name)
    HC->>F: create_model_client(model_config, secret_resolver, provider_registry)
    F->>SR: resolve(api_key_ref)
    F->>PR: get_provider(model_config.provider)
    F-->>HC: client (OpenAICompatibleClient / AnthropicClient)
    HC->>MF: ModelFacade(model_config, provider_registry, client=client)
    MF-->>HC: facade
    HC->>MF: facade.generate() / facade.generate_text_embeddings()
Loading

Reviews (2): Last reviewed commit: "Merge branch 'main' into andreatgretel/f..." | Re-trigger Greptile

@andreatgretel
Copy link
Copy Markdown
Contributor Author

Health checks pipeline is passing again on this branch: https://github.com/NVIDIA-NeMo/DataDesigner/actions/runs/23749207791

All providers passed in 36s (previously failing with exit code 2 since March 9).

Copy link
Copy Markdown
Contributor

@nabinchha nabinchha left a comment

Choose a reason for hiding this comment

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

Thanks @andreatgretel!

@andreatgretel andreatgretel merged commit a2b1c2c into main Mar 30, 2026
47 checks passed
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