https://github.com/stitchfix/hamilton logo
Join Slack
Powered by
I created <another PR> to enable ruff. The sdk uni...
# contribute
i
I created another PR to enable ruff. The sdk unit tests are failing, but I'm not sure why.
👀 2
s
could be a numpy version issue
yeah I think unit tests only get run if that part of the code base changed
looks like it’s a numpy 2.0 issue
• Alias 
np.NaN
has been removed. Use 
np.nan
instead.
would have happened on next touch of the sdk package
and it should fix it
otherwise looking good.
👍 1
i
Other incompatibilities kept popping up. I ended up limiting numpy to <2 for the sdk tests, as a temporary workaround until pyspark 3.5.2 is released (it's in rc5 atm) which fixes that problem.
👍 2
I suspect similar issues could pop up in other parts of the codebase once we start turning more stones via reformatting.
@Stefan Krawczyk would you like to merge this PR as-is and open a new one for enabling additional ruff checks? If so, I think it makes sense to merge it unsquashed, as the commits are fairly self-contained and topic-specific.
👀 1
e
Just want to chime in — we really appreciate your contributions @Iliya R!
👍 1
s
@Iliya R sure we can also note the commits and bulk add them to the git-blame-ignore file when done.
i
@Elijah Ben Izzy בכיף ;)
What can I say - I think the library is much needed and useful, and if I have the ability and time to make it a tiny bit better - why not?
❤️ 2
e
תודה רבה!
(Heh 9 years of hebrew school definitely left its mark 😆 )
🙂 1
i
I marked it as ready for review
s
done. thanks
added https://github.com/DAGWorks-Inc/hamilton/issues/1090 to track PRs related to clean up to then add to ignore file
@Iliya R is flake8 still needed?
Getting this locally now
Copy code
pre-commit run --all-files
[INFO] Initializing environment for <https://github.com/astral-sh/ruff-pre-commit>.
[INFO] Initializing environment for <https://github.com/pre-commit/pre-commit-hooks>.
[INFO] Initializing environment for <https://github.com/pycqa/flake8>.
[INFO] Installing environment for <https://github.com/astral-sh/ruff-pre-commit>.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for <https://github.com/pre-commit/pre-commit-hooks>.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for <https://github.com/pycqa/flake8>.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
ruff.....................................................................Passed
ruff-format..............................................................Passed
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
fix requirements.txt.....................................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

<unknown>:165: SyntaxWarning: invalid escape sequence '\d'
<unknown>:157: SyntaxWarning: invalid escape sequence '\d'
scripts/test_memory.py:52:55: E226 missing whitespace around arithmetic operator
scripts/test_memory.py:64:42: E226 missing whitespace around arithmetic operator
ui/sdk/src/hamilton_sdk/adapters.py:311:61: E226 missing whitespace around arithmetic operator
writeups/garbage_collection/memory_test.py:49:55: E226 missing whitespace around arithmetic operator
i
flake is needed since I haven't enabled all of its checks in ruff. Will need to be done gradually to ensure coverage is maintained. Strange that you got that error since I didn't get it locally nor did it show up in CI
👍 1
s
fresh install of flake?
🤷
t
Just want to chime in — we really appreciate your contributions Iliya R!
Agreed! I'm excited to work on a feature and start a new branch from main to benefit from these updates!
🙏 1
i
@Thierry Jean how does this affect your development? I would've guessed that minimally, since ruff is initially configured to do little more than replicate the behavior of the tools it replaces (so far it's just black and isort).
t
I manually trigger linting in my IDE to clean up the formatting, so Ruff should make everything a bit smoother! Otherwise, I'm happy to see a bit less clutter at the top of the repo!
i
Oh, I see. Ruff-lint is so fast that it has a setting (at least in vscode) to run after each keystroke. I personally enabled the formatter to run on save and changed window focus, which I find to be convenient most of the time (except when it deletes imports I just added and are still unused, for which there are workarounds).
💡 1
What I meant by my first question is one could have the ruff plugin enabled on their end and get most of these benefits even without changes to the repo, which merely tweak the enabled inspections. ...which is probably a recommendation that should be added to the project's dev docs somewhere, since otherwise the formatter & linter would only run upon committing, which skips much of their benefit.
s
okay I managed to do B. The others seems like a lot of work actually — and stylistically not sure I agree with some of them. E.g. dict() vs {} in places where I think dict is simpler to read…
i
There's a way to enable inspections on new code only, in which case existing violations will be ignored (annotations will be auto-added). Note that one doesn't have to enable an entire category each time, since individual inspections can be enabled and disabled as needed.
s
Right, but if someone touches a module, they would have to fix that entire module right? Otherwise some of it is a little “new to me”, e.g. requiring 
else
statements for the return line, with 
try except
versus it being in the middle. We could turn off those specific things — but then that’s extra work…
i
I don't think people will need to fix the entire module, since the violations will have been ignored in existing code
You saw that there are explanations for each rule on ruff's website, right?
s
Yep - I’m just reacting to having to feeling like I need to understand all that to make an informed decision. 🤔 right now our pre-commit check in CI is 
pre-commit run --all-files
so we’d have to change that?
i
Usually only modified/new files are checked, but that shouldn't really matter since existing files are expected to adhere to the rules anyway
s
i
Bottom line - no need to modify the pre-commit command imho
s
yeah — otherwise any rules you feel strongly about @Iliya R?
i
If something comes up I'll let you know, but in general, the list I included (partially commented out) is pretty much it. Not that I'm familiar with all rules or even families... Plus, they keep adding more every week...
Open in Slack
PreviousNext
Powered by