-
Notifications
You must be signed in to change notification settings - Fork 387
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
Kempner Configs #335
Kempner Configs #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gone through all of these steps yet and probably won't get to that today, but if I run into issues later I'll make a PR to update the instructions. I have a couple comments though in light of the fact that we've decided to open-source this repo:
- I don't like how we're piling up docs in the root of the repo. Can we move
Kempner.md
andLUMI.md
to adocs/
folder? - Having a
scripts/XXX-on-Y.sh
for every training config and every platform we run on is not necessary. Can we at least keep it to a single sbatch script per platform and resource requests? See what I did in https://github.com/allenai/LLM/pull/324/files withscripts/sbatch-128.sh
.
I don't think this is going to work. Look at how this plays out for the Llama-DefaultLN config: https://github.com/allenai/LLM/blob/5817f8167ce268066b8ef382306bef024d5e090c/scripts/v1_5-mix-medium-llama-on-kempner.sh#L30 I'd like to have one YAML file per model (maybe two because of the lists of files in S3), but then several configs with overrides in the scripts folder. Or maybe in some other folder, but I think we need those extra settings in the sbatch files. |
Ok, fair enough. Can we at least organize the sbatch scripts a little better? For example:
We could merge the mcli vs non-mcli yaml configs if we implemented a wildcard/glob expansion for S3.. but we can worry about that later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the general organization argument that Pete is making (but don't think it's a deal breaker) and I don't have anything extra to add, so I'm approving.
I changed the organization. Another review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to add DATA_PATH
and the other variables to the lumi scripts (like c4-*-on-lumi.sh
)? Otherwise this looks good.
I did, didn’t I?
I left the C4 ones alone. They are pretty old now. But the other ones I
modified.
…On Oct 26, 2023 at 13:51:12, Shane A ***@***.***> wrote:
***@***.**** commented on this pull request.
Do you need to add DATA_PATH and the other variables to the lumi scripts
(like c4-*-on-lumi.sh)? Otherwise this looks good.
—
Reply to this email directly, view it on GitHub
<#335 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHAYPSXLHW3I5TPP5H32T3YBLEMBAVCNFSM6AAAAAA6KWGCVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBQGYZDGOBQGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
You didn't add to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to update the mcli configs (configs/mcli/*.yaml
) to account for renaming the train configs. Other than that LGTM
Done, but we have some mcli yamls in config/, and some in scripts/? |
Oh, Ananya's stuff? I didn't see that until now. We should just move those to |
Is this good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.