fix: replace yamllint library install with exec call by nathan-weinberg · Pull Request #876 · instructlab/instructlab

nathan-weinberg · 2024-04-16T16:54:04Z

Changes

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

Description of your changes:

Removes yamllint from requirements.txt
Adds yamllint to required binaries list in README.md
Adds .yamllint file with default rules to replace in-line code, changed CLI default from yaml_rules.yaml to .yamllint to match (see https://yamllint.readthedocs.io/en/stable/configuration.html)
Updates ilab diff click arg to align with ilab generate
Updates read_taxonomy_file to exec out to yamllint instead of doing the non-compliant library import method

Some test output (note I have DEBUG log level here):

# valid taxonomy
(venv) [nathan@nathan-redhat cli (fix-lint)]$ ilab diff
Taxonomy in //home/nathan/instruct-lab/repos/taxonomy/ is valid :)
# invalid taxonomy
(venv) [nathan@nathan-redhat cli (fix-lint)]$ ilab diff
compositional_skills/extraction/abstractive/abstract/qna.yaml
DEBUG 2024-04-16 11:52:59,644 utils.py:495 Found new taxonomy files:
DEBUG 2024-04-16 11:52:59,644 utils.py:497 * compositional_skills/extraction/abstractive/abstract/qna.yaml
DEBUG 2024-04-16 11:52:59,644 utils.py:416 Using YAML rules from .yamllint
ERROR 2024-04-16 11:52:59,701 utils.py:433 Problems found in file /home/nathan/instruct-lab/repos/taxonomy/compositional_skills/extraction/abstractive/abstract/qna.yaml
21:3: [error] syntax error: expected <block end>, but found '?' (syntax)
105:3: [warning] wrong indentation: expected 4 but found 2 (indentation)
106:3: [warning] wrong indentation: expected 4 but found 2 (indentation)
Reading taxonomy failed with the following error: 3 taxonomy files with errors! Exiting.

importing yamllint directly into the cli is not an option due to license issues this maintains functionality while bringing us into compliance Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

cli/lab.py

README.md

tiran · 2024-04-16T17:49:30Z

Would it be okay to install yamllint but not import it?

If you keep yamllint in requirements.txt, then yamllint command will be installed in the virtual environments. pip install yamllint also installs $VIRTUAL_ENV/bin/yamllint, which is in the default PATH.

nathan-weinberg · 2024-04-16T17:52:45Z

Would it be okay to install yamllint but not import it?

If you keep yamllint in requirements.txt, then yamllint command will be installed in the virtual environments. pip install yamllint also installs $VIRTUAL_ENV/bin/yamllint, which is in the default PATH.

@mrutkows would you know the answer to this?

nathan-weinberg · 2024-04-16T17:57:36Z

I think the unit tests are failing as when pytest runs the .yamllint file is not getting picked up due to how it is detected

If -c is not provided, yamllint will look for a configuration file in the following locations (by order of preference):

    a file named .yamllint, .yamllint.yaml, or .yamllint.yml in the current working directory, or a parent directory (the search for this file is terminated at the user’s home or filesystem root)

    a filename referenced by $YAMLLINT_CONFIG_FILE, if set

    a file named $XDG_CONFIG_HOME/yamllint/config or ~/.config/yamllint/config, if present

We can't do the former method of inline YAML rules in the code due to the execing out - I'd like to have an explicit path to that file but not sure how to get an absolute path to the CLI root dir, would anyone know a way to do so or have an idea for another solution?

mrutkows

LGTM from a revert PoV. Unit tests need to pass of course... Thanks @nathan-weinberg for such a quick response!

cli/utils.py

mrutkows · 2024-04-16T18:13:26Z

Resolves #870 once merged.

russellb

I'm happy with it from a licensing PoV.

I have one implementation suggestion, though.

.yamllint

bjhargrave · 2024-04-16T18:25:49Z

We can't do the former method of inline YAML rules in the code due to the execing out

-d CONFIG_DATA

So can supply the inline string of the default config as a string on the command line.

usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files] [-f {parsable,standard,colored,github,auto}] [-s] [--no-warnings] [-v]
                [FILE_OR_DIR ...]

Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

Previously was vice versa Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

revert related changes to README.md Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

cli/utils.py

Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

cli/utils.py

mrutkows

Approving this necessary reversion.

cli/utils.py

anik120

Existing test cases are passing, and it's taking care of the licensing consideration. Should be good for now, I'm guessing we'll have follow ups for this later on.

nathan-weinberg requested a review from russellb April 16, 2024 16:54

nathan-weinberg requested review from Tomcli, abhi1092, afrittoli, anik120, hickeyma, jeremyeder, kelbrown20, mairin, markstur, mrutkows, soltysh, spzala and xukai92 as code owners April 16, 2024 16:54

nathan-weinberg requested a review from bjhargrave April 16, 2024 16:54

fix: replace yamllint library install with exec call

bf71840

importing yamllint directly into the cli is not an option due to license issues this maintains functionality while bringing us into compliance Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

bjhargrave suggested changes Apr 16, 2024

View reviewed changes

cli/lab.py Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

mrutkows mentioned this pull request Apr 16, 2024

Recent inclusion of yamllint causes GPL3 conflict with project Apache 2 license #870

Closed

mrutkows approved these changes Apr 16, 2024

View reviewed changes

cli/utils.py Show resolved Hide resolved

nathan-weinberg added this to the Milestone 4/18 milestone Apr 16, 2024

russellb suggested changes Apr 16, 2024

View reviewed changes

.yamllint Outdated Show resolved Hide resolved

nathan-weinberg added 3 commits April 16, 2024 13:46

Removed .yamllint file and readded inline yaml rules

c96d453

Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

Align 'lab generate' click args with original 'lab diff'

bd53179

Previously was vice versa Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

readd yamllint to requirements with version restriction and new comment

4f67552

revert related changes to README.md Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

nathan-weinberg requested a review from bjhargrave April 16, 2024 19:58

nathan-weinberg requested review from mrutkows and russellb April 16, 2024 19:58

bjhargrave suggested changes Apr 16, 2024

View reviewed changes

cli/utils.py Outdated Show resolved Hide resolved

cli/utils.py Show resolved Hide resolved

Add strict checking to all yamllint exec calls

99c7e1b

Signed-off-by: Nathan Weinberg <nweinber@redhat.com>

anik120 suggested changes Apr 16, 2024

View reviewed changes

cli/utils.py Show resolved Hide resolved

mrutkows approved these changes Apr 16, 2024

View reviewed changes

cli/utils.py Show resolved Hide resolved

anik120 approved these changes Apr 16, 2024

View reviewed changes

anik120 merged commit 3d997d0 into instructlab:main Apr 16, 2024

nathan-weinberg mentioned this pull request Apr 16, 2024

Remove yamllint from requirements.txt #879

Closed

nathan-weinberg deleted the fix-lint branch April 16, 2024 21:49

Conversation

nathan-weinberg commented Apr 16, 2024

Changes

Uh oh!

Uh oh!

Uh oh!

tiran commented Apr 16, 2024

Uh oh!

nathan-weinberg commented Apr 16, 2024

Uh oh!

nathan-weinberg commented Apr 16, 2024

Uh oh!

mrutkows left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrutkows commented Apr 16, 2024

Uh oh!

russellb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjhargrave commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrutkows left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anik120 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrutkows left a comment •

edited

Loading

bjhargrave commented Apr 16, 2024 •

edited

Loading