Create proposal for training repo#85
Create proposal for training repo#85RobotSail wants to merge 1 commit intoinstructlab:maininstructlab/dev-docs:mainfrom
Conversation
b17eac1 to
2c0c19f
Compare
nathan-weinberg
left a comment
There was a problem hiding this comment.
Overall LGTM, one comment
| The maintainers should be the folks who currently work on training. | ||
| Namely: | ||
|
|
||
| - [Aldo Pareja](https://github.com/aldopareja) | ||
| - [James Kunstle](https://github.com/orgs/instructlab/people/JamesKunstle) | ||
| - [Oleg Silkin](https://github.com/orgs/instructlab/people/RobotSail) | ||
| - [Mustafa Eyceoz](https://github.com/orgs/instructlab/people/Maxusmusti) |
There was a problem hiding this comment.
Can we create a new GitHub Team for this, something like Training Maintainers? And these folks can be defined as the initial members
There was a problem hiding this comment.
Sure, that makes sense to me.
There was a problem hiding this comment.
I don't think this made it into the doc?
russellb
left a comment
There was a problem hiding this comment.
This lgtm, though I'll hold off on approval until more folks weigh in.
Please take a look at the lining errors when you have some time.
I'd also suggest moving the file to a new docs/training/ directory, similar to the docs/cli/ directory. I want to create a similar docs/sdg/ directory. I think this organization will help.
1bef61d to
03ec16b
Compare
|
LGTM |
Signed-off-by: Oleg S <97077423+RobotSail@users.noreply.github.com>
03ec16b to
f719c6d
Compare
I made a |
leseb
left a comment
There was a problem hiding this comment.
Thanks for submitting a proposal! The PR title/Commit title are a bit confusing at first. "create proposal for training repo" is too generic, please rephrase to highlight the proposal.
| # Spelling | ||
| dictionary.dic | ||
|
|
||
| venv |
| @@ -0,0 +1,77 @@ | ||
| # Modify Repository Proposal: Training |
There was a problem hiding this comment.
How about simply docs/training/repo.md? We are in the training directory already so no need to repeat it.
|
|
||
| Today, we have training implemented for different use-cases | ||
| existing in different repos. LoRA/QLoRA training currently lives | ||
| in the [`instructlab`](https://github.com/instructlab/instructlab) |
There was a problem hiding this comment.
IIUC, this logic will have to be extracted to instructlab/training once we agree on this proposal, right?
| - [James Kunstle](https://github.com/orgs/instructlab/people/JamesKunstle) | ||
| - [Oleg Silkin](https://github.com/orgs/instructlab/people/RobotSail) | ||
| - [Mustafa Eyceoz](https://github.com/orgs/instructlab/people/Maxusmusti) |
There was a problem hiding this comment.
These 3 links return 404 for me.
|
Modulo feedback from @leseb, this LGTM. When feedback addressed I will take another look. |
cdoern
left a comment
There was a problem hiding this comment.
just some initial thoughts
| The maintainers should be the folks who currently work on training. | ||
| Namely: | ||
|
|
||
| - [Aldo Pareja](https://github.com/aldopareja) |
There was a problem hiding this comment.
I'd like to be added to this as the community train rep currently. Or @alimaredia. If we are planning on moving that code to the library, I would appreciate a part in that 🙏
There was a problem hiding this comment.
The sdg and eval repos bootstrapped access by copying the existing Backend Maintainers team. That's an alternative that could be used here.
| Rather than consolidating the logic, we can keep the existing | ||
| logic as-is. | ||
|
|
||
| This would mean that the [CLI repo](https://github.com/instructlab/instructlab) and the [training repo](https://github.com/instructlab/training) would both maintain their own implementations of training. |
There was a problem hiding this comment.
This is more what I was thinking, and this overlaps with #52 alot. Something we need to reconcile
There was a problem hiding this comment.
Does this comment mean that this alternative is what you had in mind?
|
This shouldn't be merged until it is reconciled with #52 which pre-exists this |
|
@RobotSail would you mind rebasing this? The main edit I see a need for is just reflecting that "Backend Maintainers" was used to create the initial maintainers list. Otherwise, I think we should just merge this. This work has already moved on and is well in progress. cc @cdoern since you felt this should block on #52 , though this doc doesn't really talk about CLI integration details, so I think it's OK to merge it. Let me know if there's some other conflicting info you'd like to comment on more specifically. |
hickeyma
left a comment
There was a problem hiding this comment.
Thanks @RobotSail for pushing the PR. Its looks ok if it is just covering making the existing repo into a library.
I am bit confused by "Move everything into one repo but don't design an interface" section. Can you explain what you mean here.
As @russellb said, the PR should probably be merged as the library is now in situ.
|
This happened but the doc was never merged? |
|
This pull request has been automatically marked as stale because it has not had activity within 30 days. It will be automatically closed if no further activity occurs within 7 days. |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it! |
Creates a proposal for a new training repo.
Signed-off-by: Oleg S 97077423+RobotSail@users.noreply.github.com