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.

Sign up for GitHub

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

Jump to bottom

Model/Optimizer Setup functions need moving and typehints #225

Open
Maxusmusti opened this issue Sep 25, 2024 · 7 comments · May be fixed by #264
Open

Model/Optimizer Setup functions need moving and typehints #225

Maxusmusti opened this issue Sep 25, 2024 · 7 comments · May be fixed by #264
Assignees
@malinjawi
Labels
good first issue Good for newcomers

Comments

@Maxusmusti
Copy link
Contributor

Currently in main_ds.py (which should be main.py now) there are some functions specific to model and optimizer setup that would be nice to move into a separate file. When doing this, we should also go through and make sure typehints are consistent throughout this new file, and the repo generally.
@Maxusmusti Maxusmusti mentioned this issue Sep 25, 2024
Merged
@RobotSail RobotSail self-assigned this Sep 25, 2024
@ktam3 ktam3 added the good first issue Good for newcomers label Oct 2, 2024
@malinjawi
Copy link

hey @Maxusmusti I am new to instructlab and wanted to take up this issue. Are there changes that still need to be made?

@malinjawi malinjawi linked a pull request Oct 10, 2024 that will close this issue
Open
@malinjawi
Copy link

hey @Maxusmusti @ktam3 I am new to instructlab I have raised a PR for this issue. Please let me know if there are any changes from my end. I have ran the tests with all of them passing.

@ktam3 ktam3 linked a pull request Oct 10, 2024 that will close this issue
setup_model.py and setup_optimizer.py moved to seprate files and adde… #264
Open
@ktam3
Copy link

ktam3 commented Oct 10, 2024

I've linked your PR to this issue, I'll wait for @Maxusmusti @RobotSail @JamesKunstle for their review here though. Thank you!

@Maxusmusti
Copy link
Contributor Author

Hi @malinjawi thanks for taking this on! I'll take a look and review once I'm back online 👍🏼

@malinjawi
Copy link

Hey @Maxusmusti! Thanks for the opportunity to contribute. Please let me know if there are any comments I need to address

@malinjawi
Copy link

Hey @Maxusmusti @RobotSail @JamesKunstle please if you can review my PR. I am open to solving any feedback with immediate effect.

@Maxusmusti
Copy link
Contributor Author

Hi @malinjawi in case you didn't see, some feedback was left last week, please take a look when you get a chance! The PR also requires a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@malinjawi malinjawi

Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

setup_model.py and setup_optimizer.py moved to seprate files and adde… malinjawi/training
4 participants
@Maxusmusti @RobotSail @malinjawi @ktam3