Skip to content

Conversation

@steven10a
Copy link
Collaborator

Added all of the Python content from the early access repo

  • Tested a few examples to confirm it works (hello_world, multi_bundle, agents_sdk)
  • Ran make serve-docs and verified the docs look good

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR moves all Python content from the early access repository to provide a complete guardrails SDK implementation. The changes include:

  • Complete Python SDK implementation with async/sync OpenAI client wrappers
  • Type system and runtime execution framework for guardrails
  • Comprehensive test coverage with unit and integration tests
  • Evaluation framework with benchmarking capabilities

Reviewed Changes

Copilot reviewed 125 out of 140 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
vercel.json Deployment configuration for docs building
tests/unit/*.py Unit test suite covering types, specs, runtime, and registry
tests/integration/*.py Integration tests and demo configuration
src/guardrails/types.py Core type definitions and protocols
src/guardrails/spec.py Guardrail specification and metadata models
src/guardrails/runtime.py Configuration loading and execution engine
src/guardrails/registry.py Guardrail registration and discovery system
src/guardrails/exceptions.py Exception hierarchy for error handling
src/guardrails/context.py Context management using ContextVars
src/guardrails/utils/*.py Utility modules for schema, parsing, and context validation
src/guardrails/resources/*.py OpenAI API resource wrappers with guardrails
src/guardrails/evals/*.py Comprehensive evaluation and benchmarking framework

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +83 to +86
def __post_init__(self) -> None:
"""Validate required fields and consistency."""
if "checked_text" not in self.info:
raise ValueError("GuardrailResult.info must contain 'checked_text' field")
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The documentation states that 'checked_text' is required in the info dict, but the validation logic doesn't match the docstring. The docstring says 'Must include' but doesn't explain why this field is mandatory or what it should contain.

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +44
try:
validate_dataset(path)
except ValueError as e:
logger.error("Dataset validation failed: %s", e)
raise ValueError(f"Dataset validation failed: {e}") from e
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Double error handling creates redundant validation error messages. The validate_dataset function already logs errors, so this catch-and-re-raise pattern results in duplicate logging and error wrapping.

Suggested change
try:
validate_dataset(path)
except ValueError as e:
logger.error("Dataset validation failed: %s", e)
raise ValueError(f"Dataset validation failed: {e}") from e
validate_dataset(path)

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +38
def __init__(self, output_dir: Path) -> None:
"""Initialize the visualizer.

Args:
output_dir: Directory to save generated charts
"""
self.output_dir = output_dir
self.output_dir.mkdir(parents=True, exist_ok=True)

# Set style and color palette
plt.style.use('default')
self.colors = [
'#1f77b4', '#ff7f0e', '#2ca02c', '#d62728', '#9467bd',
'#8c564b', '#e377c2', '#7f7f7f', '#bcbd22', '#17becf',
]
sns.set_palette(self.colors)
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Setting global matplotlib and seaborn styles in the constructor can affect other parts of the application. Consider using context managers or style parameters in individual plotting methods to avoid global side effects.

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +26
'.c', '.cpp', '.cs', '.css', '.doc', '.docx', '.go', '.html',
'.java', '.js', '.json', '.md', '.pdf', '.php', '.pptx',
'.py', '.rb', '.sh', '.tex', '.ts', '.txt'
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

[nitpick] The supported file types set uses mixed line formatting that makes it harder to maintain. Consider formatting each extension on its own line for better version control diffs and easier maintenance.

Suggested change
'.c', '.cpp', '.cs', '.css', '.doc', '.docx', '.go', '.html',
'.java', '.js', '.json', '.md', '.pdf', '.php', '.pptx',
'.py', '.rb', '.sh', '.tex', '.ts', '.txt'
'.c',
'.cpp',
'.cs',
'.css',
'.doc',
'.docx',
'.go',
'.html',
'.java',
'.js',
'.json',
'.md',
'.pdf',
'.php',
'.pptx',
'.py',
'.rb',
'.sh',
'.tex',
'.ts',
'.txt',

Copilot uses AI. Check for mistakes.
@gabor-openai gabor-openai requested a review from Copilot October 2, 2025 22:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 125 out of 140 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

src/guardrails/evals/core/types.py:1

  • The supported file types set is defined in the wrong file (types.py) when it should be in create_vector_store.py where it's actually used. This creates unnecessary coupling and makes the types module less focused.
"""

src/guardrails/evals/core/benchmark_calculator.py:1

  • The NaN check not value != value is confusing and potentially incorrect. The standard way to check for NaN in Python is not math.isnan(value) or not pandas.isna(value) for clearer intent and reliability.
"""

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

def __post_init__(self) -> None:
"""Validate required fields and consistency."""
if "checked_text" not in self.info:
raise ValueError("GuardrailResult.info must contain 'checked_text' field")
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The __post_init__ method validation will fail for existing code that creates GuardrailResult instances without the required 'checked_text' field. This represents a breaking change that could cause runtime errors in production code that was previously working.

Suggested change
raise ValueError("GuardrailResult.info must contain 'checked_text' field")
# Backward compatibility: insert default and warn
object.__setattr__(self, "info", {**self.info, "checked_text": ""})
logger.warning(
"GuardrailResult.info did not contain 'checked_text'; defaulting to empty string. "
"Please update code to include 'checked_text' in info."
)

Copilot uses AI. Check for mistakes.
Comment on lines +112 to +120
from sklearn.metrics import roc_curve
fpr, tpr, _ = roc_curve(y_true, y_scores)
roc_auc = np.trapz(tpr, fpr)
ax.plot(fpr, tpr, label=f'{model_name} (AUC = {roc_auc:.3f})', linewidth=2)
except Exception as e:
logger.error("Failed to calculate ROC curve for model %s: %s", model_name, e)

# Add diagonal line and customize plot
ax.plot([0, 1], [0, 1], 'k--', alpha=0.5, label='Random Classifier')
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The ROC AUC calculation using np.trapz(tpr, fpr) is incorrect. The correct calculation should be np.trapz(tpr, fpr) with reversed order or use sklearn.metrics.auc(fpr, tpr) for proper AUC computation.

Copilot uses AI. Check for mistakes.
try:
from openai import AsyncAzureOpenAI
except ImportError:
AsyncAzureOpenAI = None # type: ignore
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Setting AsyncAzureOpenAI = None after import failure creates potential runtime errors when the code later checks if not AsyncAzureOpenAI:. A more robust approach would be to use a proper sentinel value or define a stub class.

Suggested change
AsyncAzureOpenAI = None # type: ignore
class AsyncAzureOpenAI:
def __init__(self, *args, **kwargs):
raise ImportError("AsyncAzureOpenAI is not available. Please install the required dependencies.")

Copilot uses AI. Check for mistakes.
@gabor-openai gabor-openai merged commit 091a665 into main Oct 2, 2025
@steven10a steven10a deleted the dev/steven/python_split_import branch October 6, 2025 14:20
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.

3 participants