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

mergekit-mega: compound merging using multiple yaml documents in a single merge config #72

Merged
merged 12 commits into from
Jan 5, 2024

Conversation

nyxkrage
Copy link
Contributor

@nyxkrage nyxkrage commented Jan 4, 2024

This adds the a new script mergekit-mega that takes a yaml file with multiple merge configs specified as individual yaml documents and merges them in the proper order depending on interdependencies.

A couple of quirks that should probably be ironed out before merging

  1. Currently there's no special handling for circular dependencies, and I'm unsure how it would break.
  2. I dont know if the out_path in the yaml file is ideal, or if it should instead be a name, and the out_dir gets specified on the command line.
  3. The huggingface repo regex probably doesn't cover all the edge cases in its current form
  4. Unsure if just skipping over existing output directories should be the default behavior or if it should just abort, and have --force and --skip be 2 flags.

@cg123
Copy link
Collaborator

cg123 commented Jan 4, 2024

This is great! I'll look this over soon and give you some comments - think there are a few minor tweaks that would clean things up a bit. Excited to get this in.

You can ignore pre-commit complaining about the multi-document yaml file by the way - I'll change that setting.

@nyxkrage
Copy link
Contributor Author

nyxkrage commented Jan 4, 2024

Added a basic check for circular dependencies and by henky's suggestion decided on using names for the individual merges and then specifying the working directory/out_path as a cli arg.
Using names also means that the regex for checking if its a local model or a huggingface repo is no longer needed

R1702 not fixed due to the  inherently nested nature of the config format
E1120 not fixed due to arguments handled by click
R0912 not fixed due to being introduced by click
Copy link
Collaborator

@cg123 cg123 left a comment

Choose a reason for hiding this comment

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

Looks pretty much good to me! I had one nitpick, and it looks like isort wants a change or two. Those aside I'm happy to merge this in.

When #67 makes its way in there's infrastructure to do exactly the kind of graph wrangling you wrote in here, which could make things cleaner - but that's a ways off.

Thanks for the PR!

mergekit/scripts/megamerge.py Outdated Show resolved Hide resolved
@nyxkrage
Copy link
Contributor Author

nyxkrage commented Jan 5, 2024

Alright, added some docstrings mostly to please pylint ran isort and moved the part about checking if its a dependent merge or not out to a separate function, so should be good to merge!

@nyxkrage
Copy link
Contributor Author

nyxkrage commented Jan 5, 2024

Actually just realized that you can't use other models from the merge as a base model! 😅

@cg123
Copy link
Collaborator

cg123 commented Jan 5, 2024

Looks great! Thanks for putting this together.

@cg123 cg123 merged commit ebcaa04 into arcee-ai:main Jan 5, 2024
4 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.

2 participants