Add pattern checks for linting #1374
Conversation
nimbinatus
force-pushed
the
taxonomy-1369
branch
from
095906e to
eb03f4e
Compare
Signed-off-by: Laura Santamaria <[email protected]>
…tructlab#1369 Signed-off-by: Laura Santamaria <[email protected]>
Signed-off-by: Laura Santamaria <[email protected]>
nimbinatus
force-pushed
the
taxonomy-1369
branch
from
eb03f4e to
9a7d72a
Compare
|
(Also, does squash and merge not work here in this repo, so I need to squash and force push to clean up history before merging on my own?) |
| if taxonomy.errors > 0: | ||
| exit_code = 1 | ||
| if taxonomy.warnings > 0: |
There was a problem hiding this comment.
This is wrong since N errors and 1 warning means exit code 0. These 2 lines are not needed since exit_code is initialized to 0.
There was a problem hiding this comment.
Oh bugger, you're right; I'm resetting the exit code. Sorry; fixing that.
| # maintainers to address rather than block on them. We will | ||
| # revisit when other content is allowed. | ||
| qna_file_path = taxonomy.rel_path.with_name("qna.yaml") | ||
| if "knowledge" in qna_file_path.parts: |
There was a problem hiding this comment.
This may be too promiscuous since the user could put "knowledge" some where in their sub path: compositional_skills/philosophy/knowledge/qna.yaml
I think you need to look at only part 0.
There was a problem hiding this comment.
Ah, fair point. Didn't consider that edge case.
| # revisit when other content is allowed. | ||
| qna_file_path = taxonomy.rel_path.with_name("qna.yaml") | ||
| if "knowledge" in qna_file_path.parts: | ||
| qna_file_contents = parser.parse(qna_file_path).contents |
There was a problem hiding this comment.
The qna.yaml is already parsed in the taxonomy object: taxonomy.contents. Why parse it again?
There was a problem hiding this comment.
Misread the code in schema, really. Thanks for catching it.
| if "knowledge" in qna_file_path.parts: | ||
| qna_file_contents = parser.parse(qna_file_path).contents | ||
| for element in qna_file_contents["document"]["patterns"]: | ||
| if not re.match('.*.md', element): |
There was a problem hiding this comment.
I thought pdf support was added? Anyway the regex here would be
re.search("\.md$", element)
But what about the pattern folder_of_md_files/* which is a legitimate value which should not be rejected.
If you want to do more checking here, I don't think you can do it by pattern matching the yaml contents. You would need to clone the repo, find all files in the repo which match the patterns, and then check that all those files match the desired file types.
There was a problem hiding this comment.
Yes, pdf support was added in the recent release. Oversight on my part as a newbie.
Technically, the .* should match directory patterns when used with re.match() and has in the testing I've done, but you're right on the higher level that I probably shouldn't be matching file patterns (I really should be just parsing the string instead of getting into regex at all...) and should be validating the file type from the repo. Was there a reason the prior version at #1192 was dropped, or did it just lack contributor time?
| qna_file_contents["document"]["repo"]): | ||
| taxonomy.warning( | ||
| "The document repo \"%s\" needs to be a " | ||
| "GitHub-based repository.", |
There was a problem hiding this comment.
No it doesn't have to be GitHub. Any valid git repo could be used. We just expect that any such git repo can be accessed because any necessary authorization is configured.
There was a problem hiding this comment.
I've been getting conflicting information on this as I've onboarded onto the project, so I'm validating that with oversight first (left a note in the taxonomy triage channel on InstructLab Slack to get more info). I'll update this when I get an answer there.
| "GitHub-based repository.", | ||
| qna_file_contents["document"]["repo"] | ||
| ) | ||
| if not re.match( |
There was a problem hiding this comment.
This potential check is discussed here: instructlab/schema#30
If we do want to require SHA values, we should probably do that in the schema. But I am not convinced that is a great idea. Since we could allow non-SHA values that have special meanings.
Fixes #1369
Adds in three pattern checks to address issues discovered during triage. Warns as these will not break code, but rather are specific to legal requirements (best actual documentation I can find is https://github.com/instructlab/taxonomy/blob/main/README.md?plain=1#L284-L287, but this information was discussed in a triage meeting with @jjasghar and @juliadenham).
Also, hides
uv-specific files from git.