Skip to content

Conversation

@Elbehery
Copy link
Contributor

@Elbehery Elbehery commented Jan 6, 2026

What does this PR do?

This PR fixes config processing to skip registered resources when their ID fields resolve to empty/None from conditional environment variable syntax (e.g., ${env.VAR:+value}).

Changes

  • Added RESOURCE_ID_FIELDS constant listing all resource ID fields: model_id, shield_id, dataset_id, scoring_fn_id, benchmark_id, toolgroup_id
  • Modified replace_env_vars() to check if any resource ID field resolves to empty/None and skip the entire resource entry early during config processing
  • Added unit tests for the new functionality

Why is this needed?

When using conditional env var syntax like ${env.ENABLE_BENCHMARK:+my-benchmark}, if the env var is not set, the benchmark_id (or other resource ID) resolves to empty/None. Previously this would cause validation errors. Now these entries are gracefully skipped during config processing.

Fixes #4453

@Elbehery
Copy link
Contributor Author

Elbehery commented Jan 6, 2026

cc @leseb @saichandrapandraju

@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch 2 times, most recently from da5bf74 to 8b144c9 Compare January 7, 2026 13:56
@leseb
Copy link
Collaborator

leseb commented Jan 7, 2026

need to backport in 0.4.x branch

@Elbehery
Copy link
Contributor Author

Elbehery commented Jan 7, 2026

sure, shall we merge this first ?

Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

the fix needs to be in replace_env_vars not after the processing, also we need to not only handle benchmarks but other registerable ressources. this diff worked for me

diff --git a/src/llama_stack/core/stack.py b/src/llama_stack/core/stack.py
index 3ea2e8996..d573fba64 100644
--- a/src/llama_stack/core/stack.py
+++ b/src/llama_stack/core/stack.py
@@ -110,6 +110,10 @@ REGISTRY_REFRESH_INTERVAL_SECONDS = 300
 REGISTRY_REFRESH_TASK = None
 TEST_RECORDING_CONTEXT = None

+# ID fields for registered resources that should trigger skipping
+# when they resolve to empty/None (from conditional env vars like :+)
+RESOURCE_ID_FIELDS = ["model_id", "shield_id", "dataset_id", "scoring_fn_id", "benchmark_id", "toolgroup_id"]
+

 def is_request_model(t: Any) -> bool:
     """Check if a type is a request model (Pydantic BaseModel).
@@ -346,15 +350,31 @@ def replace_env_vars(config: Any, path: str = "") -> Any:
                             logger.debug(
                                 f"Skipping config env variable expansion for disabled provider: {v.get('provider_id', '')}"
                             )
-                            # Create a copy with resolved provider_id but original config
-                            disabled_provider = v.copy()
-                            disabled_provider["provider_id"] = resolved_provider_id
                             continue
                     except EnvVarError:
                         # If we can't resolve the provider_id, continue with normal processing
                         pass

-                # Normal processing for non-disabled providers
+                # Special handling for registered resources: check if ID field resolves to empty/None
+                # from conditional env vars (e.g., ${env.VAR:+value}) and skip the entry if so
+                if isinstance(v, dict):
+                    should_skip = False
+                    for id_field in RESOURCE_ID_FIELDS:
+                        if id_field in v:
+                            try:
+                                resolved_id = replace_env_vars(v[id_field], f"{path}[{i}].{id_field}")
+                                if resolved_id is None or resolved_id == "":
+                                    logger.debug(
+                                        f"Skipping resource with empty {id_field} (conditional env var not set)"
+                                    )
+                                    should_skip = True
+                                    break
+                            except EnvVarError:
+                                pass
+                    if should_skip:
+                        continue
+
+                # Normal processing
                 result.append(replace_env_vars(v, f"{path}[{i}]"))
             except EnvVarError as e:
                 raise EnvVarError(e.var_name, e.path) from None

@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch 2 times, most recently from c5f2dfb to b29e3a1 Compare January 7, 2026 22:26
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

please revise your commit message and PR title/content

@Elbehery Elbehery changed the title fix: filter benchmarks with None benchmark_id before validation fix: skip resources with empty IDs from conditional env vars in config processing Jan 8, 2026
@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch from b29e3a1 to 031f4b9 Compare January 8, 2026 09:41
@Elbehery
Copy link
Contributor Author

Elbehery commented Jan 8, 2026

updated

@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch from 031f4b9 to 39b981c Compare January 8, 2026 14:29
@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch 2 times, most recently from 4d14190 to 0141bdb Compare January 8, 2026 17:44
…g processing

Added handling in replace_env_vars() to skip registered resources when their
ID fields (model_id, shield_id, dataset_id, scoring_fn_id, benchmark_id,
toolgroup_id) resolve to empty/None from conditional env var syntax like
${env.VAR:+value}.

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
@Elbehery Elbehery force-pushed the 20260106_allow_empty_banchmarkId branch from 0141bdb to 6871912 Compare January 9, 2026 10:06
@leseb leseb merged commit 589c0d0 into llamastack:main Jan 9, 2026
34 checks passed
@leseb
Copy link
Collaborator

leseb commented Jan 9, 2026

@Mergifyio backport release-0.4.x

@mergify
Copy link

mergify bot commented Jan 9, 2026

backport release-0.4.x

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jan 9, 2026
…g processing (#4455)

## What does this PR do?

This PR fixes config processing to skip registered resources when their
ID fields resolve to empty/None from conditional environment variable
syntax (e.g., `${env.VAR:+value}`).

### Changes

- Added `RESOURCE_ID_FIELDS` constant listing all resource ID fields:
`model_id`, `shield_id`, `dataset_id`, `scoring_fn_id`, `benchmark_id`,
`toolgroup_id`
- Modified `replace_env_vars()` to check if any resource ID field
resolves to empty/None and skip the entire resource entry early during
config processing
- Added unit tests for the new functionality

### Why is this needed?

When using conditional env var syntax like
`${env.ENABLE_BENCHMARK:+my-benchmark}`, if the env var is not set, the
`benchmark_id` (or other resource ID) resolves to empty/None. Previously
this would cause validation errors. Now these entries are gracefully
skipped during config processing.

Fixes #4453

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
(cherry picked from commit 589c0d0)
@Elbehery Elbehery deleted the 20260106_allow_empty_banchmarkId branch January 9, 2026 10:44
leseb pushed a commit that referenced this pull request Jan 9, 2026
…g processing (backport #4455) (#4475)

## What does this PR do?

This PR fixes config processing to skip registered resources when their
ID fields resolve to empty/None from conditional environment variable
syntax (e.g., `${env.VAR:+value}`).

### Changes

- Added `RESOURCE_ID_FIELDS` constant listing all resource ID fields:
`model_id`, `shield_id`, `dataset_id`, `scoring_fn_id`, `benchmark_id`,
`toolgroup_id`
- Modified `replace_env_vars()` to check if any resource ID field
resolves to empty/None and skip the entire resource entry early during
config processing
- Added unit tests for the new functionality

### Why is this needed?

When using conditional env var syntax like
`${env.ENABLE_BENCHMARK:+my-benchmark}`, if the env var is not set, the
`benchmark_id` (or other resource ID) resolves to empty/None. Previously
this would cause validation errors. Now these entries are gracefully
skipped during config processing.

Fixes #4453<hr>This is an automatic backport of pull request #4455 done
by [Mergify](https://mergify.com).

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
Co-authored-by: Mustafa Elbehery <melbeher@redhat.com>
franciscojavierarceo pushed a commit to franciscojavierarceo/llama-stack that referenced this pull request Jan 16, 2026
…g processing (llamastack#4455)

## What does this PR do?

This PR fixes config processing to skip registered resources when their
ID fields resolve to empty/None from conditional environment variable
syntax (e.g., `${env.VAR:+value}`).

### Changes

- Added `RESOURCE_ID_FIELDS` constant listing all resource ID fields:
`model_id`, `shield_id`, `dataset_id`, `scoring_fn_id`, `benchmark_id`,
`toolgroup_id`
- Modified `replace_env_vars()` to check if any resource ID field
resolves to empty/None and skip the entire resource entry early during
config processing
- Added unit tests for the new functionality

### Why is this needed?

When using conditional env var syntax like
`${env.ENABLE_BENCHMARK:+my-benchmark}`, if the env var is not set, the
`benchmark_id` (or other resource ID) resolves to empty/None. Previously
this would cause validation errors. Now these entries are gracefully
skipped during config processing.

Fixes llamastack#4453

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
JayDi11a pushed a commit to JayDi11a/llama-stack that referenced this pull request Jan 23, 2026
…g processing (llamastack#4455)

## What does this PR do?

This PR fixes config processing to skip registered resources when their
ID fields resolve to empty/None from conditional environment variable
syntax (e.g., `${env.VAR:+value}`).

### Changes

- Added `RESOURCE_ID_FIELDS` constant listing all resource ID fields:
`model_id`, `shield_id`, `dataset_id`, `scoring_fn_id`, `benchmark_id`,
`toolgroup_id`
- Modified `replace_env_vars()` to check if any resource ID field
resolves to empty/None and skip the entire resource entry early during
config processing
- Added unit tests for the new functionality

### Why is this needed?

When using conditional env var syntax like
`${env.ENABLE_BENCHMARK:+my-benchmark}`, if the env var is not set, the
`benchmark_id` (or other resource ID) resolves to empty/None. Previously
this would cause validation errors. Now these entries are gracefully
skipped during config processing.

Fixes llamastack#4453

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow None or Empty benchmark_id similar to the behavior with provider_id.

3 participants