Skip to content

Remove fastchat dependency#98

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
danmcp:removefastchatdep
Sep 23, 2024
Merged

Remove fastchat dependency#98
mergify[bot] merged 1 commit into
instructlab:mainfrom
danmcp:removefastchatdep

Conversation

@danmcp

@danmcp danmcp commented Aug 13, 2024

Copy link
Copy Markdown
Contributor

There were only a couple of remaining calls to fastchat. Models we don't interact with have been removed from support.

pylint version has been updated to correspond to cli and to allow the type | None syntax.

@danmcp danmcp added the hold In-progress PR. Tag should be removed before merge. label Aug 13, 2024
@danmcp

danmcp commented Aug 13, 2024

Copy link
Copy Markdown
Contributor Author

@alimaredia FYI

@danmcp

danmcp commented Aug 13, 2024

Copy link
Copy Markdown
Contributor Author

@dhellmann Note this will remove the fastchat dependency

@dhellmann

Copy link
Copy Markdown

@dhellmann Note this will remove the fastchat dependency

Ack, thanks for the heads-up.

@danmcp danmcp removed the hold In-progress PR. Tag should be removed before merge. label Aug 15, 2024
@jaideepr97

jaideepr97 commented Aug 19, 2024

Copy link
Copy Markdown
Contributor

@danmcp are we confident we don't need to rely on fastchat for any future developments to the code that we are importing from them?
This seems great from a dependency reduction point of view, but I'm guessing we would essentially be "freezing" the borrowed code (unless we plan to develop it in a different direction)

@danmcp

danmcp commented Aug 19, 2024

Copy link
Copy Markdown
Contributor Author

@danmcp are we confident we don't need to rely on fastchat for any future developments to the code that we are importing from them?

I wish I could predict the future and answer this question :). We had already included the bulk of the fastchat code we needed within eval. This change largely includes the remainder of customizations from xukai92

lm-sys/FastChat@main...xukai92:FastChat:ilab

that aren't included in FastChat anyway and leaves out support for most model type conversations. Given the amount of customization we are doing already, and the amount the code is tied to OpenAI vs FastChat, I think we are stuck maintaining our own version that works with the versions of OpenAI and vllm and models we need to support.

I think the biggest risk comes to changes we need to make to the evaluation conversation with the model and judge and less to the core code.

This seems great from a dependency reduction point of view, but I'm guessing we would essentially be "freezing" the borrowed code (unless we plan to develop it in a different direction)

There is already a decent bit of divergence with xukai92's changes, adding support for a new OpenAI version, support for custom judges, and support for merge-system-user-message.

This is definitely a good question to ask. This particular PR doesn't increase the risk very much vs what we had already done, but the overall problem of how to maintain everything long term still exists.

@nathan-weinberg

Copy link
Copy Markdown
Member

@Mergifyio refresh

@mergify

mergify Bot commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

refresh

✅ Pull request refreshed

@mergify mergify Bot added CI/CD Affects CI/CD configuration one-approval dependencies Pull requests that update a dependency file labels Aug 28, 2024
@nathan-weinberg
nathan-weinberg requested a review from a team September 12, 2024 14:55
@mergify

mergify Bot commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @danmcp please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Sep 13, 2024
There were only a couple of remaining calls to fastchat.  Models we don't interact with have been removed from support.

Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@mergify mergify Bot removed the needs-rebase label Sep 13, 2024
@mergify mergify Bot removed the one-approval label Sep 23, 2024
@mergify
mergify Bot merged commit 53d6abf into instructlab:main Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Affects CI/CD configuration dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants