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

Prompt Crafter json loads fix for numbers #1527

Merged
merged 6 commits into from
Oct 23, 2023
Merged

Prompt Crafter json loads fix for numbers #1527

merged 6 commits into from
Oct 23, 2023

Conversation

SamGos93
Copy link
Contributor

Fix the failure when output pattern has a number and the json loads fails.
Removing the try/except logic for json loads as for chat models, we would always need the role to be added.

@SamGos93 SamGos93 requested a review from a team as a code owner October 20, 2023 07:55
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 07:55 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 07:55 — with GitHub Actions Inactive
@mercerchen
Copy link

@SamGos93 Can you point me to the checksum test pipeline? Tried to find it in the checks but couldn't find it. I am assuming this would be breaking for this test.
image

mercerchen
mercerchen suggested changes Oct 20, 2023
View reviewed changes
Copy link

@mercerchen mercerchen left a comment

Choose a reason for hiding this comment

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

prompt_factory line 327 to 332 would be breaking for the screenshoted case I put in the issue

@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 20:52 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 20:52 — with GitHub Actions Inactive
@SamGos93
Copy link
Contributor Author

@SamGos93 Can you point me to the checksum test pipeline? Tried to find it in the checks but couldn't find it. I am assuming this would be breaking for this test. image

@mercerchen : I have updated the logic. Can you please check?

@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 21:04 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 21:04 — with GitHub Actions Inactive
@SamGos93 SamGos93 changed the title Sagoswami/prompt crafter int json fix Prompt Crafter json loads fix for numbers Oct 20, 2023
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 21:12 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 20, 2023 21:12 — with GitHub Actions Inactive
@mercerchen
Copy link

@SamGos93 Can you tell me thne name of the test pipeline? the code looks good just want to see the tests passing

@SamGos93
Copy link
Contributor Author

@SamGos93 Can you tell me thne name of the test pipeline? the code looks good just want to see the tests passing

It is probably this https://github.com/Azure/azureml-assets/actions/runs/6592757956/job/17914044989?pr=1527. We would know once it completes/ fails.

@github-actions
Copy link

github-actions bot commented Oct 20, 2023 •
edited
Loading

Test Results for assets-test

205 tests   202 ✔️  1h 37m 35s ⏱️
    1 suites      1 💤
    1 files        2 ❌

For more details on these failures, see this check.

Results for commit 6967698.

♻️ This comment has been updated with latest results.

@SamGos93 SamGos93 temporarily deployed to Testing October 21, 2023 06:25 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 21, 2023 06:25 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 23, 2023 05:21 — with GitHub Actions Inactive
@SamGos93 SamGos93 temporarily deployed to Testing October 23, 2023 08:07 — with GitHub Actions Inactive
@SamGos93 SamGos93 merged commit 21a3016 into main Oct 23, 2023
@SamGos93 SamGos93 deleted the sagoswami/prompt_crafter_int_json_fix branch October 23, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mercerchen mercerchen mercerchen requested changes

@alpatots alpatots alpatots approved these changes

@iamrk04 iamrk04 iamrk04 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SamGos93 @mercerchen @iamrk04 @alpatots