[stale] [poc] Fix/fix-escape-chars-in-llm-as-s-judge-and-prompts (continuation) #2912
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
GitHub CI seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR continues stale work on fixing escape characters in LLM-as-judge and prompts. The changes primarily involve:
- Renaming database models and API models from
Testset/TestsetDBtoTestSet/TestSetDBfor consistency - Removing deprecated evaluation-related database models and converters
- Simplifying environment variable handling by using
os.getenvdefault parameters instead oforoperators - Refactoring workflow service architecture including error handling, DTOs, and service registry
- Cleaning up evaluator and evaluation-related code including flags, versioning, and JIT migration logic
- Updating test data to use consistent naming conventions
Reviewed Changes
Copilot reviewed 127 out of 1365 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/oss/tests/pytest/testsets/test_testsets_basics.py |
Updated test data to use "Test Set Name" instead of "Testset Name" |
api/oss/tests/pytest/testsets/test_testcases_basics.py |
Updated test data to use "Test Set Name" and "Another Test Set Name" |
api/oss/tests/pytest/testsets/legacy/test_testsets_basics.py |
Added trailing slashes to upload endpoint paths |
api/oss/tests/manual/workflows/interface.py |
Added new manual test interface for workflows with 764 lines of test code |
api/oss/tests/legacy/old_tests/variants_main_router/*.py |
Renamed TestsetDB to TestSetDB across multiple test files |
api/oss/tests/legacy/old_tests/models.py |
Updated imports to use TestSetDB instead of TestsetDB |
api/oss/tests/legacy/conftest.py |
Updated project name from "Default Project" to "default" and simplified Gemini model list |
api/oss/src/utils/logging.py |
Refactored logging configuration, removed PID from logs, and cleaned up handler setup |
api/oss/src/utils/env.py |
Simplified environment variable retrieval using os.getenv default parameters |
api/oss/src/utils/common.py |
Removed unused is_uuid7 utility function |
api/oss/src/services/*.py |
Updated function signatures and removed deprecated evaluation-related code |
api/oss/src/routers/*.py |
Updated API endpoints, removed query parameters, and simplified response models |
api/oss/src/models/db_models.py |
Renamed TestsetDB to TestSetDB and removed deprecated evaluation models |
api/oss/src/models/api/*.py |
Updated model names and removed deprecated fields |
api/oss/src/core/workflows/*.py |
Major refactoring of workflow service architecture with new DTOs and error handling |
api/oss/src/core/services/*.py |
Added new service registry and v0 implementations for evaluators |
api/oss/src/core/evaluations/*.py |
Simplified evaluation flags, removed version constants, and updated service methods |
api/oss/src/core/evaluators/*.py |
Refactored evaluator flags and simplified data transfer logic |
api/oss/src/dbs/postgres/tracing/*.py |
Simplified query logic and removed unnecessary conditionals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CRISP_WEBSITE_ID: str = os.getenv("CRISP_WEBSITE_ID", "") or "" | ||
| STRIPE_API_KEY: str = os.getenv("STRIPE_API_KEY", "") or "" | ||
| STRIPE_WEBHOOK_SECRET: str = os.getenv("STRIPE_WEBHOOK_SECRET", "") or "" | ||
| POSTHOG_API_KEY: str = os.getenv("POSTHOG_API_KEY") |
There was a problem hiding this comment.
Missing default value for POSTHOG_API_KEY. Line 55 should use os.getenv(\"POSTHOG_API_KEY\", \"\") to match the pattern used elsewhere and avoid potential None values.
Suggested change
| POSTHOG_API_KEY: str = os.getenv("POSTHOG_API_KEY") | |
| POSTHOG_API_KEY: str = os.getenv("POSTHOG_API_KEY", "") |
Comment on lines
+485
to
491
| workflow_variant = await self.query_workflow_variants( | ||
| project_id=project_id, | ||
| # | ||
| workflow_ref=workflow_ref, | ||
| ) | ||
|
|
||
| if not workflow: | ||
| return None | ||
|
|
||
| workflow_ref = Reference( | ||
| id=workflow.id, | ||
| slug=workflow.slug, | ||
| ) | ||
|
|
||
| workflow_variant = await self.fetch_workflow_variant( | ||
| project_id=project_id, | ||
| workflow_refs=[workflow_ref], | ||
| # | ||
| workflow_ref=workflow_ref, | ||
| windowing=Windowing(limit=1, order="descending"), | ||
| ) |
There was a problem hiding this comment.
Missing null check after querying workflow_variants. If the query returns an empty list, accessing workflow_variant[0] at lines 497-498 will raise an IndexError.
| # Match both single and double curly brace variables | ||
| pattern = r"\{+([a-zA-Z_][a-zA-Z0-9_.]*)\}+" | ||
| # log.info(f"Variable detection using pattern: {pattern}") | ||
| log.info(f"Variable detection using pattern: {pattern}") |
There was a problem hiding this comment.
This line was previously commented out and should remain commented or removed. Uncommenting it will produce excessive logging output during normal operation.
Suggested change
| log.info(f"Variable detection using pattern: {pattern}") |
junaway
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[POC][Stale] Fix/fix-escape-chars-in-llm-as-s-judge-and-prompts (continuation)