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.

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

Refactor Xassist, Test Framework #161

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

tharun571
Copy link
Contributor

Description

  • Added xassist method to handle special cases.
  • Updated OpenAI call method.
  • Removed unnecessary line in testing framework.

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 43.75000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.67%. Comparing base (baba783) to head (2268c60).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/xmagics/xassist.cpp 43.75% 18 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
- Coverage   81.83%   80.67%   -1.17%     
==========================================
  Files          19       19              
  Lines         837      864      +27     
  Branches       91       93       +2     
==========================================
+ Hits          685      697      +12     
- Misses        152      167      +15     
Files with missing lines Coverage Δ
src/xmagics/xassist.cpp 75.10% <43.75%> (-5.38%) ⬇️

... and 2 files with indirect coverage changes

Files with missing lines Coverage Δ
src/xmagics/xassist.cpp 75.10% <43.75%> (-5.38%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/xmagics/xassist.cpp Outdated Show resolved Hide resolved
src/xmagics/xassist.cpp Outdated Show resolved Hide resolved
src/xmagics/xassist.cpp Outdated Show resolved Hide resolved
src/xmagics/xassist.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/xmagics/xassist.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491
Copy link
Collaborator

Just curious as to what exactly the refactor is doing here ? Maybe explain through a simple example ?

@tharun571
Copy link
Contributor Author

Just curious as to what exactly the refactor is doing here ? Maybe explain through a simple example ?

I was going through whatever code I wrote and checked for possible bugs. This PR takes care of scenarios when the prompt has special characters or anything involving disrupting the JSON to call the LLM.

}
return escaped;
}

Copy link
Collaborator

@anutosh491 anutosh491 Oct 17, 2024

Choose a reason for hiding this comment

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

This PR takes care of scenarios when the prompt has special characters or anything involving disrupting the JSON to call the LLM.

Hmm, so should there be a test required in this PR to check this claim ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so should there be a test required in this PR to check this claim ?

I did try but got in a predicament where escape_special_cases("""{}") == "\"\"{}" yields false but works in the kernel. So I tested them manually and pushed the code

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@vgvassilev vgvassilev merged commit a3b5cff into compiler-research:main Oct 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants