- Notifications
You must be signed in to change notification settings - Fork17
Moving all the python content over#1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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 orlearn 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") |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
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.
| try: | ||
| validate_dataset(path) | ||
| except ValueError as e: | ||
| logger.error("Dataset validation failed: %s", e) | ||
| raise ValueError(f"Dataset validation failed: {e}") from e |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
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.
| try: | |
| validate_dataset(path) | |
| exceptValueErrorase: | |
| logger.error("Dataset validation failed: %s",e) | |
| raiseValueError(f"Dataset validation failed:{e}")frome | |
| validate_dataset(path) |
| 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) |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
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.
| '.c', '.cpp', '.cs', '.css', '.doc', '.docx', '.go', '.html', | ||
| '.java', '.js', '.json', '.md', '.pdf', '.php', '.pptx', | ||
| '.py', '.rb', '.sh', '.tex', '.ts', '.txt' |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
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.
| '.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', |
There was a problem hiding this 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 increate_vector_store.pywhere 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 != valueis confusing and potentially incorrect. The standard way to check for NaN in Python isnot math.isnan(value)ornot pandas.isna(value)for clearer intent and reliability.
"""Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn 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") |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
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 createsGuardrailResult instances without the required 'checked_text' field. This represents a breaking change that could cause runtime errors in production code that was previously working.
| raiseValueError("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." | |
| ) |
| 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') |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The ROC AUC calculation usingnp.trapz(tpr, fpr) is incorrect. The correct calculation should benp.trapz(tpr, fpr) with reversed order or usesklearn.metrics.auc(fpr, tpr) for proper AUC computation.
| try: | ||
| from openai import AsyncAzureOpenAI | ||
| except ImportError: | ||
| AsyncAzureOpenAI = None # type: ignore |
CopilotAIOct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SettingAsyncAzureOpenAI = None after import failure creates potential runtime errors when the code later checksif not AsyncAzureOpenAI:. A more robust approach would be to use a proper sentinel value or define a stub class.
| AsyncAzureOpenAI=None# type: ignore | |
| classAsyncAzureOpenAI: | |
| def__init__(self,*args,**kwargs): | |
| raiseImportError("AsyncAzureOpenAI is not available. Please install the required dependencies.") |
Added all of the Python content from the early access repo
make serve-docsand verified the docs look good