-
-
Notifications
You must be signed in to change notification settings - Fork 554
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
Replace pre-commit in the CI #1666
base: main
Are you sure you want to change the base?
Conversation
@MaxHalford Do you remember why you added kwargs handling in Lines 145 to 146 in 5531ae5
I'm a bit puzzled by it, no dataset from River yields any additional argument. |
Hey 👋 I believe it was for progressive validation... But I'd need to check deeper. Do tests pass if you remove it? |
I may have found why: some datasets like MovieLens100K have an optional parameter to unpack attributes. river/river/datasets/movielens100k.py Lines 52 to 56 in 5531ae5
As you wrote in an example, this mechanism is used by recommender models. All good then. |
It looks like the tests timed out while fetching a remote dataset. I didn't touch that! |
Indeed, it was for the user_id and item_id fields in the recsys models. Good catch! |
Because pre-commit only checks edited files, side effects can cause errors to appear in other files and go unnoticed. On the contrary, the CI's role is to guarantee the soundness of the entire codebase; only checking what changed is insufficient.
This PR replaces pre-commit with an explicit call to the linters (in CI only, pre-commit is still available as a hook).
This process discovered previously silent typing errors.
If anything, we lose pre-commit's interface to select file types: the files must now be given as command arguments.
Closes #1665