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

offshoot gen_test_data() from very long generate_data() #15

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

makelinux
Copy link
Contributor

Function generate_data is huge and messy.

Refactor it.

@russellb
Copy link
Member

russellb commented Jun 5, 2024

Thank you for posting the PR to this repo!

The changes look good, though it would make my life a little easier if I hold off on merging it until I can remove the original code from instructlab/instructlab. I'm hoping that's done this week.

src/instructlab/sdg/generate_data.py Outdated Show resolved Hide resolved
@russellb
Copy link
Member

Thanks for the updates.

The changes look good, though it's highlighting the lack of integration testing in this repo. I'm going to look into that now.

@russellb
Copy link
Member

I got an e2e workflow enabled on this repo in #33. I'm going to rebase this PR to trigger it to run.

@russellb
Copy link
Member

@Mergifyio rebase

Function generate_data() is huge and messy. Refactor it.
Move code around `json.dump` to gen_test_data()
because it is under loop
`for instruction_data_entry in instruction_data`
by accidentally.

Code is just moved, without internal changes.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Function generate_data() is huge and messy.

Refactor it.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
Copy link
Contributor

mergify bot commented Jun 24, 2024

rebase

✅ Branch has been successfully rebased

Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

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

lgtm, and the e2e test workflow passed

@russellb russellb merged commit cba3a62 into instructlab:main Jun 24, 2024
7 checks passed
@makelinux makelinux deleted the gen_test_data branch August 22, 2024 09:31
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