-
Notifications
You must be signed in to change notification settings - Fork 309
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.
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
[Evals] Refactor tasks, add linting and CI #47
Changes from 13 commits
aa47191
ee8d0f4
0cf3548
f48b5dc
3e624af
859f023
d877bee
faaf293
c18ad7b
b7d9532
8b9c67e
c767708
995f1a5
7bafae1
829cb4c
484df2d
3188aeb
612ecaf
77b8ba3
52a4ce7
d21d68e
2069df7
05b1653
55b670c
f2d88e9
a9a9380
d3dde5d
9d64fd1
d55d2a8
840006f
4cdeab0
8d564a3
fc1087e
6e2e979
7449117
04ead2a
84c9617
e032b47
7e99d60
3f5ff02
79d12a2
c8a8d63
aa87124
eab138a
07c21f9
3d6942f
c2944fe
8a39701
503ea62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
set -e | ||
|
||
# Get tools directory path relative to git root | ||
TOOLS_DIR=$(git rev-parse --show-toplevel)/skythought/tools | ||
# Only run pre-commit if changes are in tools/ | ||
# Run pre-commit from tools/ directory to use linting rules in this directory | ||
if git diff --cached --name-only | grep "^skythought/tools/"; then | ||
cd $TOOLS_DIR; | ||
pre-commit run --files $(git diff --cached --name-only | grep "^skythought/tools/") --config .pre-commit-config.yaml | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
repos: | ||
- repo: https://github.com/astral-sh/ruff-pre-commit | ||
rev: v0.6.9 | ||
hooks: | ||
- id: ruff | ||
args: [ --fix, --exit-non-zero-on-fix ] | ||
|
||
# Black needs to be ran after ruff with --fix | ||
- repo: https://github.com/psf/black | ||
rev: 23.3.0 | ||
hooks: | ||
- id: black |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not in this PR but for better structure of the codebase. We should take these scripts and maybe put them in the following refactored structure:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agreed let's postpose this higher level folder re-org for later. I've now moved the linting and pre-commit hooks to the top level repo and ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should put these linter stuff at the top of level of this repo. Did you have any reason for not putting it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to apply these to the
train
sub-folder let's just exclude that from the precommit / format runs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LMK if this doesn't make sense - the main reason I kept it separate is because the
train/
folder is a Llamafactory repo fork. It looks likeskythought/tools
will be a package in itself focused on evals.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I moved this to the top-level repo. Also added a setup.py which installes our evaluation package at
skythought/skythought_evals
(renamed fromtools
) asskythought_evals