Skip to content

fix: replace yamllint library install with exec call #876

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 16, 2024
Merged

fix: replace yamllint library install with exec call #876

merged 5 commits into from
Apr 16, 2024

Conversation

nathan-weinberg
Copy link
Member

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.

@nathan-weinberg
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
bjhargrave requested changes Apr 16, 2024
View reviewed changes
@tiran
Copy link
Contributor

tiran commented Apr 16, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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 mrutkows mentioned this pull request Apr 16, 2024
Closed
mrutkows
mrutkows approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@mrutkows mrutkows left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@mrutkows
Copy link
Contributor

Resolves #870 once merged.

@nathan-weinberg nathan-weinberg added this to the Milestone 4/18 milestone Apr 16, 2024
russellb
russellb requested changes Apr 16, 2024
View reviewed changes
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.

I'm happy with it from a licensing PoV.

I have one implementation suggestion, though.

@bjhargrave
Copy link
Contributor

bjhargrave commented Apr 16, 2024 •
edited
Loading

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 ...]

@nathan-weinberg
Removed .yamllint file and readded inline yaml rules
c96d453 
Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
@nathan-weinberg
Align 'lab generate' click args with original 'lab diff'
bd53179 
Previously was vice versa

Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
@nathan-weinberg
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>
bjhargrave
bjhargrave requested changes Apr 16, 2024
View reviewed changes
@nathan-weinberg
Add strict checking to all yamllint exec calls
99c7e1b 
Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
anik120
anik120 suggested changes Apr 16, 2024
View reviewed changes
mrutkows
mrutkows approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@mrutkows mrutkows left a comment

Choose a reason for hiding this comment

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

Approving this necessary reversion.

anik120
anik120 approved these changes Apr 16, 2024
View reviewed changes
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.

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.

@anik120 anik120 merged commit 3d997d0 into instructlab:main Apr 16, 2024
@nathan-weinberg nathan-weinberg mentioned this pull request Apr 16, 2024
Closed
@nathan-weinberg nathan-weinberg deleted the fix-lint branch April 16, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjhargrave bjhargrave bjhargrave requested changes

@mairin mairin Awaiting requested review from mairin

@kelbrown20 kelbrown20 Awaiting requested review from kelbrown20

@jeremyeder jeremyeder Awaiting requested review from jeremyeder

@abhi1092 abhi1092 Awaiting requested review from abhi1092

@soltysh soltysh Awaiting requested review from soltysh

@xukai92 xukai92 Awaiting requested review from xukai92

@markstur markstur Awaiting requested review from markstur

@hickeyma hickeyma Awaiting requested review from hickeyma

@afrittoli afrittoli Awaiting requested review from afrittoli

@spzala spzala Awaiting requested review from spzala

@Tomcli Tomcli Awaiting requested review from Tomcli

@russellb russellb Awaiting requested review from russellb

+2 more reviewers

@mrutkows mrutkows mrutkows approved these changes

@anik120 anik120 anik120 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Milestone 4/18
Development

Successfully merging this pull request may close these issues.

Recent inclusion of yamllint causes GPL3 conflict with project Apache 2 license
6 participants
@nathan-weinberg @tiran @mrutkows @bjhargrave @russellb @anik120