Skip to content

fix(python-helpers): honor environment configuration precedence - #2103

Open
sylvesterkaczmarek wants to merge 1 commit into
NVIDIA:mainfrom
sylvesterkaczmarek:sylvesterkaczmarek/fix/environment-config-precedence
Open

sylvesterkaczmarek wants to merge 1 commit into
NVIDIA:mainfrom
sylvesterkaczmarek:sylvesterkaczmarek/fix/environment-config-precedence

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Sep 25, 2026 •

Copy link
Copy Markdown

TL;DR

Honor environment-variable precedence in the Python container helper without evaluating an unused model-config fallback. Valid environment overrides currently raise KeyError when the corresponding model-config fields are absent.

Additional Details

os.environ.get(name, fallback) evaluates the fallback before checking the environment. In get_config_value, that fallback indexes the nested Triton model configuration, so an environment value alone cannot supply a parameter when a partial model config is passed.

Read an existing environment value first, including an explicitly empty string. Only consult model_config when the environment value is absent. Preserve the existing fallback lookup and KeyError details when neither source provides a value. The function signature and return types are unchanged.

This is a local Python helper correction. It adds no request handling, logging of values, network calls, telemetry, or service configuration. Runtime dependencies and NOTICE files are unchanged. The subtree's test instructions now describe the added pytest suite.

For the Reviewer

The production change is confined to get_config_value in src/libraries/python/nv-cloud-function-helpers/nv_cloud_function_helpers/nvcf_container/helpers.py. The new tests are in that package's tests/test_config_value.py.

All open PR file lists were checked for overlap; none modified this helper subtree. Issue #2102 was opened on 25 September 2026 and rechecked as unassigned with no other claimant or competing fix before submission. The patch is based on main 3414c033.

For QA

From src/libraries/python/nv-cloud-function-helpers, with the package and pytest installed:

python3 -m pytest -q tests
python3 -m build
  • All 20 regression cases pass. Seven fail and 13 controls pass on the original implementation.
  • Coverage includes incomplete fallback dictionaries, present and empty environment values, normal model-config fallback, missing-value errors, unused-fallback avoidance, environment updates, and input preservation.
  • Source distribution and wheel builds succeed. The suite passes against the built wheel outside the checkout on macOS arm64 with Python 3.10 and 3.11. Package import paths were verified.
  • All 20 cases also pass against the installed wheel in Linux arm64/Python 3.11 with container networking disabled.
  • New-test Black formatting, Python correctness lint checks, and git diff --check pass. The commit is signed off and its signature is verified by GitHub.

Validation is scoped to this standalone helper package, following its local test guidance. The wider NVCF monorepo, Bazel service targets, live Triton containers, and deployed NVCF were not tested. The package's existing missing-README source-distribution warning remains. No real credentials were read and no service or cluster was started or modified. Service-level integration remains for maintainer QA.

Issues

Fixes #2102.

Checklist

  • I am familiar with the contributing guidelines.

Summary by CodeRabbit

  • Bug Fixes

    • Configuration values now use the model configuration when the corresponding environment variable is absent, and honor environment-variable overrides, including empty values.
    • Missing configuration paths without a usable fallback now raise a KeyError instead of returning a default.
  • Documentation

    • Added instructions for running tests and clarified that they use local fixtures without requiring a running service.
    • Documented the expectation to add regression tests alongside helper changes.

Fixes NVIDIA#2102. Add regression and compatibility coverage.

Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/nvcf/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bf583cf5-ebfb-4c1f-97cf-26b2a5483dbb

📥 Commits

Reviewing files that changed from the base of the PR and between 3414c03 and dff1c16.

📒 Files selected for processing (3)
  • src/libraries/python/nv-cloud-function-helpers/AGENTS.md
  • src/libraries/python/nv-cloud-function-helpers/nv_cloud_function_helpers/nvcf_container/helpers.py
  • src/libraries/python/nv-cloud-function-helpers/tests/test_config_value.py

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

get_config_value now checks an environment variable before reading model configuration when that variable exists. Added tests cover overrides, fallback values, missing configuration errors, and environment changes. The helper package instructions now describe how to run the tests.

Changes

Configuration override behavior

Layer / File(s) Summary
Environment and model configuration lookup
src/libraries/python/nv-cloud-function-helpers/nv_cloud_function_helpers/nvcf_container/helpers.py
The helper returns the environment value when the variable exists. Otherwise, it reads the configured model value. Tests cover empty overrides, fallback values, missing configuration paths, and changing environment values.
Regression test instructions
src/libraries/python/nv-cloud-function-helpers/tests/test_config_value.py, src/libraries/python/nv-cloud-function-helpers/AGENTS.md
Adds tests for the lookup behavior and documents the pytest command and local fixture setup.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to dff1c

The environment override change preserves the checked missing-value behavior. No issue identified here needs resolution before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses the required Conventional Commits format, includes the required scope for the fix type, and accurately describes the environment configuration precedence bug fix.
Linked Issues check ✅ Passed Issue #2102 requires environment precedence, including an empty value, and preservation of fallback lookup and missing-value errors. get_config_value checks value_name in os.environ before indexin…
Out of Scope Changes check ✅ Passed The changes stay within Issue #2102. The helper change implements the precedence fix. The new tests provide regression coverage. The AGENTS.md change documents the package test command and the require…
Full details: Docstring Coverage

Explanation

Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

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.

Python helper evaluates missing model configuration despite an environment override

1 participant