-
Notifications
You must be signed in to change notification settings - Fork 87
Add Prompt dataclass with initial methods (JUD-2082) #562
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from 1 commit
90cfb2c
a7e2803
a82f167
8bd2173
d2d8dcb
2a99789
419370e
58a2848
9be3be3
aab38de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,96 @@ | ||||||||||
from typing import List, Optional | ||||||||||
import os | ||||||||||
from judgeval.api import JudgmentSyncClient | ||||||||||
from judgeval.exceptions import JudgmentAPIError | ||||||||||
from dataclasses import dataclass, field | ||||||||||
import re | ||||||||||
from string import Template | ||||||||||
|
||||||||||
|
||||||||||
def push_prompt( | ||||||||||
name: str, | ||||||||||
prompt: str, | ||||||||||
tags: List[str], | ||||||||||
judgment_api_key: str = os.getenv("JUDGMENT_API_KEY") or "", | ||||||||||
organization_id: str = os.getenv("JUDGMENT_ORG_ID") or "", | ||||||||||
) -> tuple[str, Optional[str]]: | ||||||||||
client = JudgmentSyncClient(judgment_api_key, organization_id) | ||||||||||
try: | ||||||||||
r = client.prompts_insert( | ||||||||||
payload={"name": name, "prompt": prompt, "tags": tags} | ||||||||||
) | ||||||||||
return r["commit_id"], r["parent_commit_id"] | ||||||||||
|
return r["commit_id"], r["parent_commit_id"] | |
return r["commit_id"], r.get("parent_commit_id") |
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.
key should always be there but added anyways
Outdated
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 function fetch_prompt
is missing a return type hint. Based on its usage and the prompts_fetch
method it calls, the return type should be PromptFetchResponse
. Adding type hints improves code clarity and allows for better static analysis. You will also need to import PromptFetchResponse
from judgeval.api.api_types
.
): | |
) -> "PromptFetchResponse": |
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.
+1
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.
added
Outdated
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.
style: Missing return type annotation - should specify return type for consistency with other functions
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/judgeval/prompts/prompt.py
Line: 44:44
Comment:
**style:** Missing return type annotation - should specify return type for consistency with other functions
How can I resolve this? If you propose a fix, please make it concise.
Outdated
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.
logic: The regex pattern only captures word characters (\w+) - variables with hyphens, dots, or other characters won't be matched
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/judgeval/prompts/prompt.py
Line: 136:136
Comment:
**logic:** The regex pattern only captures word characters (\w+) - variables with hyphens, dots, or other characters won't be matched
How can I resolve this? If you propose a fix, please make it concise.
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.
changed to more general
Outdated
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.
While if not tags:
works, it's more idiomatic and explicit in Python to check for None
with if tags is None:
. This avoids potential confusion if an empty list is passed intentionally and you wanted to treat it differently from None
(though in this case the outcome is the same).
if not tags: | |
tags = [] | |
if tags is None: | |
tags = [] |
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.
changed
Outdated
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 parent_commit_id
key is not guaranteed to be in the prompt_config
dictionary as it is marked as NotRequired
in the PromptFetchResponse
type definition. Accessing it directly with prompt_config["parent_commit_id"]
will raise a KeyError
if the key is absent. You should use prompt_config.get("parent_commit_id")
for safe access.
parent_commit_id=prompt_config["parent_commit_id"], | |
parent_commit_id=prompt_config.get("parent_commit_id"), |
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.
key should always be present (may return none), but added anyways
Uh oh!
There was an error while loading. Please reload this page.