Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: pushing new manifest without metadata update #907

Merged
merged 11 commits into from
Aug 11, 2024
Merged

fix: pushing new manifest without metadata update #907

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc736d7
fix: pushing new manifest without metadata update
madams0013 Aug 2, 2024
b605427
lint
madams0013 Aug 2, 2024
d5fa3b4
make bool non optional
madams0013 Aug 2, 2024
d995041
limt
madams0013 Aug 3, 2024
22f1772
Merge branch 'main' into maddy/patch-push-prompt-python
madams0013 Aug 6, 2024
ce0c8f4
just false
madams0013 Aug 6, 2024
248bb56
change negation to any
madams0013 Aug 6, 2024
281043d
test
madams0013 Aug 6, 2024
6e09e09
prettier
madams0013 Aug 6, 2024
0b26e82
merge
madams0013 Aug 11, 2024
41bf6a4
Merge branch 'main' of github.com:langchain-ai/langsmith-sdk into mad…
madams0013 Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions python/langsmith/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5036,7 +5036,7 @@ def create_prompt(
description: Optional[str] = None,
readme: Optional[str] = None,
tags: Optional[Sequence[str]] = None,
is_public: bool = False,
is_public: Optional[bool] = False,
Copy link
Collaborator

@hinthornw hinthornw Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what's the need for this? / does "none" mean anything diff than false?

) -> ls_schemas.Prompt:
"""Create a new prompt.

Expand Down Expand Up @@ -5074,7 +5074,7 @@ def create_prompt(
"description": description or "",
"readme": readme or "",
"tags": tags or [],
"is_public": is_public,
"is_public": is_public or False,
madams0013 marked this conversation as resolved.
Show resolved Hide resolved
}

response = self.request_with_retries("POST", "/repos/", json=json)
Expand Down Expand Up @@ -5350,13 +5350,20 @@ def push_prompt(
"""
# Create or update prompt metadata
if self._prompt_exists(prompt_identifier):
self.update_prompt(
prompt_identifier,
description=description,
readme=readme,
tags=tags,
is_public=is_public,
)
if not (
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: easier to read if we flip the conditions:

if (all this stuff is none) then create prompt, else update

Copy link
Contributor Author

@madams0013 madams0013 Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we still need to do this check after checking if prompt exists because if prompt doesnt' exist we just want to create it

if prompt exists AND none of these things are set, skip the call to update prompt

Copy link
Collaborator

@hinthornw hinthornw Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just saying it's hard to read a double negative (not x is None vs. if x); the logic is right.

I find both of the following easier to read:

if any(
    parent_commit_hash is None
    and is_public is None
    and description is None
    and readme is None
    and tags is None
):
    self.create_prompt(
        prompt_identifier,
        is_public=is_public,
        description=description,
        readme=readme,
        tags=tags,
    )
else:
    self.update_prompt(
        prompt_identifier,
        description=description,
        readme=readme,
        tags=tags,
        is_public=is_public,
    )

or

if any (
    parent_commit_hash is not None
    and is_public is not None
    and description is not None
    and readme is not None
    and tags is not None
):
    self.update_prompt(
        prompt_identifier,
        description=description,
        readme=readme,
        tags=tags,
        is_public=is_public,
    )
else:
    self.create_prompt(
        prompt_identifier,
        is_public=is_public,
        description=description,
        readme=readme,
        tags=tags,
    )

parent_commit_hash is None
and is_public is None
and description is None
and readme is None
and tags is None
):
self.update_prompt(
prompt_identifier,
description=description,
readme=readme,
tags=tags,
is_public=is_public,
)
else:
self.create_prompt(
prompt_identifier,
Expand Down
13 changes: 12 additions & 1 deletion python/tests/integration_tests/test_prompts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,11 @@ def test_create_commit(
langsmith_client.delete_prompt(prompt_name)


def test_push_prompt(langsmith_client: Client, prompt_template_3: PromptTemplate):
def test_push_prompt(
langsmith_client: Client,
prompt_template_3: PromptTemplate,
prompt_template_2: ChatPromptTemplate
):
prompt_name = f"test_push_new_{uuid4().hex[:8]}"
url = langsmith_client.push_prompt(
prompt_name,
Expand Down Expand Up @@ -444,6 +448,13 @@ def test_push_prompt(langsmith_client: Client, prompt_template_3: PromptTemplate
assert updated_prompt.description == "Updated prompt"
assert not updated_prompt.is_public
assert updated_prompt.num_commits == 1

# test updating prompt manifest but not metadata
url = langsmith_client.push_prompt(
prompt_name,
object=prompt_template_2,
)
assert isinstance(url, str)

langsmith_client.delete_prompt(prompt_name)

Expand Down
Loading