Skip to content

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Apr 11, 2024

Closes #786

Which issue is resolved by this Pull Request:
Resolves #786

Description of your changes:

This PR:

  • Moves the helper functions from generate_data.py to utils,py. Several other packges (eg diff) are using these helper functions(eg read_taxonomy), and they were calling generate.<helper_function>, which is incorrect.
  • Introduces tests for the helper functions in utils.py, but is restricted to only the helper functions used during knowledge workflow.
  • Introduces test for generate_data with knowledge workflow.

Copy link
Contributor

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

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

There is a lot going on in this PR. It would be easier to review if multiple commits were used with each commit focused on a specific task (refactoring code, refactoring tests, adding new tests, etc.)

anik120 added 4 commits April 14, 2024 10:58
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
@anik120
Copy link
Contributor Author

anik120 commented Apr 14, 2024

There is a lot going on in this PR. It would be easier to review if multiple commits were used with each commit focused on a specific task (refactoring code, refactoring tests, adding new tests, etc.)

@xukai92 @bjhargrave dissected the changes into multiple commits, PTAL!

Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Copy link
Contributor

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

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

Thanks for breaking up into several commits!

Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Copy link
Contributor

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

@anik120 anik120 merged commit cbfbb1b into instructlab:main Apr 16, 2024
@hickeyma hickeyma deleted the test-knowledge branch April 16, 2024 15:44
jgato pushed a commit to jgato/instructlab that referenced this pull request Jun 21, 2024
Description: Removing old file spelling error file

Signed-off-by: Kelly Brown <kelbrown@redhat.com>
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.

Unit tests for knowledge cli changes
3 participants