Skip to content

ci(cloudbuild): skip cloudbuild for documentation-only PRsIgnore docs cloudrun - #195

Open
g-lynnzee wants to merge 3 commits into
mainfrom
ignore-docs-cloudrun
Open

ci(cloudbuild): skip cloudbuild for documentation-only PRsIgnore docs cloudrun#195
g-lynnzee wants to merge 3 commits into
mainfrom
ignore-docs-cloudrun

Conversation

@g-lynnzee

Copy link
Copy Markdown
Collaborator

No description provided.

tested: fixes used to run local CUJs for me.
Summary Implements an automated transport fallback mechanism for how Evalbench calls QueryData to seamlessly support unreleased/pre-release protocol buffer fields (such as ) without requiring end users to modify tools.yaml. The goal is to enable developing and releasing pre-public features in context engineering agent, such as to private preview customers.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a documentation-only PR check in CI to skip smoke tests, updates local execution documentation, and implements a multi-tiered REST API fallback mechanism (Production and Staging REST) in the evaluation runner to handle unreleased or non-public proto fields. The review feedback highlights several critical improvements: wrapping the datasource_references dictionary in a list since it is a repeated field, explicitly removing api_endpoint from the configuration when it is None to prevent state leakage, removing generic Python exceptions from the proto error patterns to avoid false positives, and adding warning logs when fallback tiers fail to improve troubleshooting.

Comment on lines +61 to +64
if isinstance(datasource_ref, dict):
query_context_dict = {
"datasource_references": datasource_ref
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The datasource_references field in QueryDataContext is a repeated field. When serializing to a dictionary representation, its value must be a list of dictionaries rather than a single dictionary. If datasource_ref is a dictionary, it should be wrapped in a list.

Suggested change
if isinstance(datasource_ref, dict):
query_context_dict = {
"datasource_references": datasource_ref
}
if isinstance(datasource_ref, dict):
query_context_dict = {
"datasource_references": [datasource_ref]
}

Comment on lines +282 to +284
cfg["use_rest_api"] = use_rest_api
if api_endpoint:
cfg["api_endpoint"] = api_endpoint

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If api_endpoint is None (as in the 'Production REST API' tier), the existing api_endpoint key is not removed from cfg. If a previous run or tier set api_endpoint in the configuration file, it will persist and leak into subsequent runs that expect to use the default production endpoint. Explicitly removing the key when api_endpoint is None prevents this configuration leakage.

Suggested change
cfg["use_rest_api"] = use_rest_api
if api_endpoint:
cfg["api_endpoint"] = api_endpoint
cfg["use_rest_api"] = use_rest_api
if api_endpoint:
cfg["api_endpoint"] = api_endpoint
else:
cfg.pop("api_endpoint", None)

Comment on lines +247 to +257
_PROTO_FIELD_ERROR_PATTERNS = (
"attributeerror",
"typeerror",
"valueerror",
"protocol message",
"unknown field",
"invalid field",
"has no attribute",
"querydatacontext",
"datasourcereferences",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Including generic Python exception names like attributeerror, typeerror, and valueerror in _PROTO_FIELD_ERROR_PATTERNS can cause false positives. Any standard Python error in the evaluation run will trigger the REST API fallback and ultimately mask the real error under a generic 'unreleased or non-public QueryData feature' message. Removing these generic exception names prevents masking unrelated bugs, as proto-specific errors will still be caught by more specific patterns like querydatacontext or datasourcereferences.

_PROTO_FIELD_ERROR_PATTERNS = (
    "protocol message",
    "unknown field",
    "invalid field",
    "querydatacontext",
    "datasourcereferences",
)

Comment on lines +335 to +338
code, output = _exec_evalbench(cmd)
if code == 0:
logger.info(f"Evaluation completed successfully via {tier_name}.")
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When a fallback tier fails with a non-zero exit code, the failure is silently ignored and the loop proceeds to the next tier. Logging a warning with the exit code and a snippet of the output when a tier fails makes troubleshooting much easier if all tiers eventually fail.

Suggested change
code, output = _exec_evalbench(cmd)
if code == 0:
logger.info(f"Evaluation completed successfully via {tier_name}.")
return
code, output = _exec_evalbench(cmd)
if code == 0:
logger.info(f"Evaluation completed successfully via {tier_name}.")
return
logger.warning(
f"{tier_name} failed with exit code {code}. Output snippet:\n{output[:200]}"
)

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.

1 participant