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

[BFCL] add ibm-granite-20b-functioncallling model #525

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

MayankAgarwal
Copy link
Contributor

This PR adds the granite-20b-functioncalling model to the BFCL.

This PR only adds the model handler. The leaderboard should be updated in a separate PR.

@HuanzhiMao
Copy link
Collaborator

Thank you for the PR @MayankAgarwal ! We will review and test it, and update the leaderboard accordingly.

Copy link
Collaborator

@HuanzhiMao HuanzhiMao left a comment

Choose a reason for hiding this comment

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

Hey @MayankAgarwal,

I noticed that you intentionally removed the language-specific hint we included at the end of the model prompt here. I'm wondering what's the reasoning behind this change?

In my opinion, this removal could negatively impact the model's performance, especially in the Java and JavaScript test categories. However, we are fine with the removal as it won't unfairly benefit the model. I just want to bring this to your attention.

The rest of the PR looks great!

@MayankAgarwal
Copy link
Contributor Author

MayankAgarwal commented Jul 15, 2024

Hi @HuanzhiMao,

Thank you for reviewing and approving the PR.

We chose to remove the language-specific prompt augmentation because of the following reasons:

  1. Since we unified the input and output format to JSON for this model, we were not sure of including any language-specific hint in the prompt. I believe that unless the model outputs code in that specific language (say Java or JavaScript in the case of BFCL), there should be a separate process to convert the function-calling output from a model to that specific language, rather than the model itself generating language-compliant code in JSON/XML data format.
  2. We did not add the language-specific hint in the prompt while experimenting with other tasks and datasets. Thus, to maintain consistency we did not include it in this PR.
  3. While building the model handler, we experimented with both adding and removing the language-specific hint and we did not see a significant difference in the performance.
  4. I think some model handlers in the BFCL code do not use the language-specific augmented prompt, such as FireworkAIHandler, DatabricksHandler.

Owing to these reasons, we decided to remove the language-specific hint. I hope these sound reasonable to you.

If there are no changes from your side, please merge the PR. I would love to see the model on the leaderboard.

Thanks again for reviewing the PR!

@ShishirPatil
Copy link
Owner

Thank you for the PR @MayankAgarwal and delighted to welcome IBM models into the BFCL!

@ShishirPatil ShishirPatil merged commit 95b0fbc into ShishirPatil:main Jul 17, 2024
ShishirPatil pushed a commit that referenced this pull request Jul 20, 2024
… PR Merge (#536)

On July 16th, PR #516 and PR #512 were merged first. They introduced
fixes that should be applied to all model handlers. Shortly after, on
the same day, PR #525 was merged. The new model handler introduced in PR
#525 is missing the fixes from the previous two merged PRs (it wasn't
synced accordingly). This PR addresses this issue by applying the
necessary fixes to the new model handler.
ShishirPatil pushed a commit that referenced this pull request Jul 24, 2024
…#539)

This PR updates the leaderboard to reflect the changes in score due to
the following PR merge.

- #516 
- #522 
- #525 
- #536 

The score for the following category are affected:
- executable_parallel_multiple
- Java (FC models only)
- JavaScript (FC models only)

Consequently, the aggregated also need to be updated: 
- Simple Func AST
- AST Summary
- Overall Acc

<img width="980" alt="image"
src="https://github.com/user-attachments/assets/04ea5265-34ed-4fb4-a2fb-5bc47affa8f7">
<img width="1076" alt="image"
src="https://github.com/user-attachments/assets/e37201e1-4cd7-43e8-b37a-5198f4809e87">


---
Aggregates

<img width="913" alt="image"
src="https://github.com/user-attachments/assets/85b0d681-ffae-42b5-af0d-e7963129bef2">
<img width="809" alt="image"
src="https://github.com/user-attachments/assets/04d0de7a-ead7-4e5c-99e4-931864d65e82">
<img width="773" alt="image"
src="https://github.com/user-attachments/assets/7df8e885-bd68-4910-896f-a27dcbfefbce">
aw632 pushed a commit to vinaybagade/gorilla that referenced this pull request Aug 22, 2024
This PR adds the
[granite-20b-functioncalling](https://huggingface.co/ibm-granite/granite-20b-functioncalling)
model to the BFCL.

This PR only adds the model handler. The leaderboard should be updated
in a separate PR.
aw632 pushed a commit to vinaybagade/gorilla that referenced this pull request Aug 22, 2024
… PR Merge (ShishirPatil#536)

On July 16th, PR ShishirPatil#516 and PR ShishirPatil#512 were merged first. They introduced
fixes that should be applied to all model handlers. Shortly after, on
the same day, PR ShishirPatil#525 was merged. The new model handler introduced in PR
ShishirPatil#525 is missing the fixes from the previous two merged PRs (it wasn't
synced accordingly). This PR addresses this issue by applying the
necessary fixes to the new model handler.
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.

3 participants