Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

fix: Improve diff and taxonomy YAML logging#756

Merged
anik120 merged 14 commits intoinstructlab:maininstructlab/instructlab:mainfrom
rafvasq:improve-diffCopy head branch name to clipboard
Apr 1, 2024
Merged

fix: Improve diff and taxonomy YAML logging#756
anik120 merged 14 commits intoinstructlab:maininstructlab/instructlab:mainfrom
rafvasq:improve-diffCopy head branch name to clipboard

Conversation

@rafvasq
Copy link
Contributor

@rafvasq rafvasq commented Mar 26, 2024

Resolves #732
cc. @anik120 @nathan-weinberg

Description of your changes:

  • Adds, updates, and improves diff tests
  • Updates main README to replace lab list and lab check with new lab diff command
  • Silences lab diff --quiet, supplying exit codes without any logging
  • lab diff now prints list of modified YAMLs and validates them, throwing exception if not valid
  • Defaults yaml_rules to None to avoid a FileNotFound exception if user doesn't supply a custom yaml config

rafvasq added 4 commits March 25, 2024 17:38
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
rafvasq added 2 commits March 26, 2024 20:42
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
README.md Outdated Show resolved Hide resolved
# Third Party
import git

TEST_VALID_YAML = """created_by: rafael-vasquez
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this change is necessary, can you explain the motivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experienced an exception somewhere when running tests where lab diff would complete but with code 1. It was hidden, but I traced an exception coming from read_taxonomy_file around here because the taxonomy file doesn't have those headers to parse. Hopefully I didn't miss something, but passing a valid YAML fixed the return code (and I figured is better than using an arbitrary string).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Though the contents of that YAML imply it's actually invalid so maybe we could update that?
@aesteve-rh I believe you originally wrote this case so appreciate your feedback here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was on PTO so didn't see this on time.

Yes, makes sense to me too :) Thanks @rafvasq for the update!

tests/test_lab_diff.py Show resolved Hide resolved
cli/generator/generate_data.py Show resolved Hide resolved
cli/generator/generate_data.py Show resolved Hide resolved
cli/generator/generate_data.py Show resolved Hide resolved
rafvasq added 2 commits March 27, 2024 15:23
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

Thank you @rafvasq! Left a few comments regarding documentation but functionally looks great!!

We can merge this once you've address the doc comments 🎉

README.md Outdated Show resolved Hide resolved
cli/generator/generate_data.py Show resolved Hide resolved
cli/lab.py Outdated Show resolved Hide resolved
rafvasq added 4 commits April 1, 2024 09:47
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@rafvasq rafvasq requested a review from anik120 April 1, 2024 13:53
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

Thanks @rafvasq !!

@anik120 anik120 merged commit 1172805 into instructlab:main Apr 1, 2024
@rafvasq rafvasq deleted the improve-diff branch April 1, 2024 15:05
],
)
self.assertIn(f"Taxonomy in {self.taxonomy.root} is valid :)", result.output)
self.assertIn("", result.output)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always evaluated as True, isn't it?

Shouldn't it be assertEquals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, and it's being addressed in #776

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.

lab diff improvement

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.