#Let's talk about pre-commit... again?

1 messages · Page 1 of 1 (latest)

marsh glade
#

Can we remove this line at the top of the pre-commit?

default_language_version:
  python: "3.8"
sand knoll
#

Why?

#

I thought we added this to keep things consistent

marsh glade
#

I'm struggling to get it to work properly in my env

sand knoll
#

oO

#

Intersting

#

For me it was the same without that and adding it fixed the problem 😄

marsh glade
#

haha

#

ok, so maybe it's a config issue for me

sand knoll
#

What problems is it causing for you?

marsh glade
#

well, now that i'm looking

#

it's not a pre-commit issue

#

make lint gives me a ton of errors

#

it runs this

# 
.PHONY: type-check
type-check: mypy pyright                            ## Run all type checking
#

is see the problem

#

it doesn't ahve the env prefix

#

so it's not using the mypy installed in the venv

#

which is my issue

#

it's using the system 3.11 interpreter for mypy and pyright

#

I was incorrectly thinking this was pre-commit related

#

well, pyright has the env check

#

actually all of them do

# 
.PHONY: mypy
mypy:                                               ## Run mypy
    @echo "=> Running mypy"
    @$(ENV_PREFIX)dmypy run
    @echo "=> mypy complete"

.PHONY: mypy-nocache
mypy-nocache:                                       ## Run Mypy without cache
    @echo "=> Running mypy without a cache"
    @$(ENV_PREFIX)dmypy run -- --cache-dir=/dev/null
    @echo "=> mypy complete"

.PHONY: pyright
pyright:                                            ## Run pyright
    @echo "=> Running pyright"
    @$(ENV_PREFIX)pyright
    @echo "=> pyright complete"
#

here was the tail of the last time i ran make lint

# 
  /home/oracle/.pyenv/versions/3.11.5/lib/python3.11/typing.py:3413:23 - error: "tuple" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.11.5/lib/python3.11/typing.py:3413:29 - error: "type" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.11.5/lib/python3.11/typing.py:3415:6 - error: Variable not allowed in type expression (reportGeneralTypeIssues)
13699 errors, 73 warnings, 0 informations 
=> pyright complete
sand knoll
#

Interesting

#

Hmm

#

have you tried a clean re-install of your pre-commit env?

marsh glade
#

doing that now

#

same outcome

sand knoll
#

Huh

marsh glade
#

maybe my python env is bad

#

let me re-install that

sand knoll
#

I always do pyenv local 3.8.x -> create venv -> install deps -> activate venv -> pre-commit install

#

That usually works

#

Idk, I'm not a huge fan of the current workflow tbh :/

#

Still better than the poetry dependency issues tho 👀

marsh glade
#

yeah, i agree.

#

ok, this is probably just user error

#

make install was re-installing with 3.11

#

no, that wasn't it either

#

/home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1971:6 - error: "property" is not defined (reportUndefinedVariable)
/home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1973:33 - error: "bool" is not defined (reportUndefinedVariable)
/home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1976:6 - error: "property" is not defined (reportUndefinedVariable)
/home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1978:27 - error: Variable not allowed in type expression (reportGeneralTypeIssues)
14228 errors, 65 warnings, 0 informations
=> pyright complete

polar wind
#

hopefully I'm not hijacking

#

make lint only runs two pre-commit checks now

#

pretty sure its from adding the pre-push default stage b/c when I try to explicitly run the ruff hook I get: "No hook with id ruff in stage pre-commit"

inland summit
#

Are the changes in litestar#2705 making it harder or easier for non .venv venvs to utilize the makefile? I almost never use .venv for litestar projects because the way we have everthing set up. just wondering

grand spearBOT
marsh glade
#

Easier.

sand knoll
polar wind
#

found it - there's a --hook-stage flag which must default to something that doesn't have pre-push in it

#

--hook-stage {commit-msg,post-checkout,post-commit,post-merge,post-rewrite,pre-commit,pre-merge-commit,pre-push,pre-rebase,prepare-commit-msg,manual}
The stage during which the hook is fired. One of commit-msg, post-checkout, post-commit, post-merge, post-rewrite, pre-commit, pre-merge-commit, pre-push, pre-rebase, prepare-commit-msg, manual

lone kindle
#

How about we add all the pre commit hooks as both pre-commit and pre-push and then whoever doesn't want to run the hooks on the commiting could use git commit -m "some message" --no-verify?

NOTE: I'm biased towards keeping them in the pre-commit stage itself because I rely on them in my workflow 😛

lone kindle
polar wind
#

it's one of the options but my guess is that if you don't provide any value it is prob the same as --hook-stage pre-commit... i've already spent more time on this than I want to though so 🤷

lone kindle
#

Yes it is.

#

There's a value you can set in .pre-commit-config.yaml that will automatically install the different hooks.

#

Hold on. I think I have it in one of my config files.

polar wind
#

this isn't really a matter of installing the hooks though

lone kindle
#

And then we can just set the default_stages as pre-commit and pre-push.

polar wind
#

the issue was that pre-commit run --all-files was only running a couple of the hooks

lone kindle
#

Oh oops you're right.

#

Then maybe we need to take advantage of pre-commit run --hook-stage pre-commit?

#

Or rather, if it's only going to have pre-push, then it would need to be pre-comm it run --hook-stage pre-push.

#

I think this requires us to set the stage of each of the hooks as pre-push as well.

polar wind
#

i think that's what @marsh glade's PR did - but I backed it out (obviously too hastily)

lone kindle
#

I don't think that PR set pre-push as an included stage for each hook though.

#

The default is almost always pre-commit and I believe that's the default for ruff and others.

polar wind
#

I'm sure you are right - so what does default_stages do (also, telling me to RTFM is a valid response here :D)?

lone kindle
#

It tells pre-commit on which stages to run. So, I think we have a hook for the conventional commit. The stage for this is commit-msg.

#

But this only applies to hooks which have not set any value for stages.

#

I'm assuming some mismatch here is why some of the hooks weren't running in the CI.

polar wind
#

default_stages

(optional: default (all stages)) a configuration-wide default for the stages property of hooks. This will only override individual hooks that do not set stages.

#

stages

(optional) selects which git hook(s) to run for. See Confining hooks to run at certain stages.

#

a comment in the code reads:
# manual is not invoked by any installed git hook. See #719

#

so maybe we just needed to set default_stages: [pre-push, manual]?

#

ahhh nope

grand spearBOT
#

pre_commit/main.py lines 76 to 82

parser.add_argument(
    '--hook-stage',
    choices=clientlib.STAGES,
    type=clientlib.transform_stage,
    default='pre-commit',
    help='The stage during which the hook is fired.  One of %(choices)s',
)
polar wind
#

this is the --hook-stage argument for pre-commit run - and default is pre-commit stage

#

so if we add default_stages: [pre-push] then we need to change the run commands that we use in places to $ pre-commit run --all-files --hook-stage=pre-push

#

actually: $ pre-commit run --all-files --hook-stage=pre-push --hook-stage=pre-commit because there are a couple of hooks that still run on the pre-commit stage even after setting pre-push as default

#

I'm happy to revert my revert for adding this, but I'll leave it as is unless you guys want me to make the above changes too

sand knoll
#

This may be a sacrilegious question but.. Are we sure we actually need pre-commit..?

#

Couldn't we just have a make lint and make type-check command and that's it?

#

Sounds like it could save us a lot of trouble

#

Regarding the workflow, I don't know about yours, but I am like @polar wind in that I don't actually run pre-commit on every commit; I just run it when I push

inland summit
#

It sounds like you guys should stop hurting my and guacs beautiful pre-commit because we actually do use it pre-commit 🥹

sand knoll
#

Using tools the way they are intended?!

inland summit
sand knoll
#

😄

sand knoll
#

Seriously though, that's why I made that suggestion. I think before we start forcing pre-commit to do something it wasn't meant to do, it's better to just use something else

#

There has to be a way around this though

inland summit
#

I dont quite understand what the issue is so far, im trying to grok

sand knoll
#

Basically @marsh glade changed pre-commit so it would run not pre-commit, but pre-push

#

Which some people liked

#

😛

inland summit
#

this sounds like for people that like that, they shouldnt need/use pre-commit

sand knoll
#

But as it turns out, that breaks our CI

inland summit
#

just use the makefile 😄

sand knoll
inland summit
#

oh 😄

sand knoll
#

We do have pre-commit however

inland summit
#

okay we use 2 pre-commit files

sand knoll
#

And maintaining two sets of linting/type-checking scripts sounds meh

inland summit
#

one for the cool kids

#

one for us

sand knoll
#

😄

marsh glade
#

So, pre-commit is actually cooperating with me it's the second section of make lint that's causing grief i think

#

pyright doesn't seem to use the correct python no matter what

sand knoll
#

Couldn't we just, instead of using pre-commit install, have a script that installs a git hook, which runs pre-commit run on pre-push?

#

And then if you don't want your pre-commit to run pre-commit, you just use that

marsh glade
#

well, i don't even run pre-commit install usually

#

and would prefer it not installed

sand knoll
#

I always disable it as well

#

I really liked the idea of it running pre-push tho

marsh glade
#

yeah, my main gripe with pre-commit install is that it forces me to make my commits meet every standard

#

when it should only be when it's pushed back to the main repo where that should matter

sand knoll
#

Well it makes sense if you're not using the squash workflow

#

Then you want your individual commits to adhere to all the rules

#

But since we don't, there's really no need for it

inland summit
marsh glade
#

we don't use "pre-commit install"

#

not we don't use precommit

#

and i'd argue we'd make it worse again 😉

#

(clearly somethng needs to change though)

inland summit
#

if you dont install the hooks whats the point i dont get it? wouldnt that just be the same as make lint

marsh glade
#

yeah, but that's the thing

#

make lint doesn't work for me

#

which is the reason for this post

polar wind
grand spearBOT
# polar wind eh hem: https://github.com/litestar-org/litestar/pull/2705#discussion_r139864475...
peterschutt

@cofin I backed this change out because it caused us to only run a couple of the hooks in CI.

It feels like the rules should only be applied when you are pushing something back into the main repo.

Agree - this is what I do but have manually managed the process by not installing the hook at all and running make lint once when I'm ready.

Just to experiment I created this as litestar/.git/hooks/pre-push and it seems to work:

#!/bin/sh

pdm run --venv 3.8 make pre-commit

...although I get warnings like:

Inside an active virtualenv /home/peter/.local/share/pdm/venvs/litestar-dj-FOhMr-3.8, reusing it.

Set env var PDM_IGNORE_ACTIVE_VENV to ignore it.

in the output - so maybe specifying the venv isn't necessary.

Comment on litestar-org/litestar#2705 ·
polar wind
#

we don't use pre-commit in our internal projects - I just have a shell script that runs a bunch of tools by calling $ ./scripts/tests - but we don't run anywhere near the number of tools that litestar does

polar wind
#

I've never had a problem with it, so trying to understand the difference in our workflows

marsh glade
polar wind
#

do you have a .pdm-python file?

marsh glade
#

yeah, it points the venv in ./.venv

polar wind
#

and is that 3.8?

marsh glade
#

yep

polar wind
#

then I'm out of ideas

#

when you run $ make lint, what is the PDM output at the top? E.g., mine says: Inside an active virtualenv /home/peter/.local/share/pdm/venvs/litestar-dj-FOhMr-3.8, reusing it.

marsh glade
# 
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1968:25 - error: Variable not allowed in type expression (reportGeneralTypeIssues)
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1968:34 - error: "str" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1971:6 - error: "property" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1973:33 - error: "bool" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1976:6 - error: "property" is not defined (reportUndefinedVariable)
  /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py:1978:27 - error: Variable not allowed in type expression (reportGeneralTypeIssues)
14317 errors, 65 warnings, 0 informations 
=> pyright complete
#

that's the end of make lint there's a huge amount of errors above that

# 
❯ make lint
=> Running pre-commit process
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
=> Pre-commit complete
=> Running mypy
Success: no issues found in 652 source files
=> mypy complete
=> Running pyright

then there will be a bunch of errors after this

polar wind
#

str is not defined 😲

# 
(litestar-3.8) peter@pop-os:~/PycharmProjects/litestar$ make lint
=> Running pre-commit process
Inside an active virtualenv /home/peter/.local/share/pdm/venvs/litestar-dj-FOhMr-3.8, reusing it.
Set env var PDM_IGNORE_ACTIVE_VENV to ignore it.
marsh glade
#

this is no the correct venv though: /home/oracle/.pyenv/versions/3.8.18/lib/python3.8/typing.py

#

well, i guess it's the base of the venv

inland summit
#

seems like its using pyenvs python instead of the venv python

polar wind
#

and why is it type checking the python sauce?

#

if you first run $ $(pdm venv activate) that activates the venv shell right?

marsh glade
# 
❯ pdm venv activate
source /home/oracle/Code/Starlite/litestar/.venv/bin/activate
❯ which python3
~/Code/Starlite/litestar/.venv/bin/python3
polar wind
#

you need to wrap it in $(...)

#

so $ $(pdm venv activate)

marsh glade
#

Looks the same to me

# 
❯ $(pdm venv activate)
❯ which python3
~/Code/Starlite/litestar/.venv/bin/python3
#

this is the correct python though

#

if i run make lint after that, i get the errors you see above

polar wind
# marsh glade Looks the same to me

yes, and I'm not sure why that is yet, but without $(...) you aren't executing anything, pdm venv activate just echo's the text that you see after it in the first example

#

any other tooling that might be modifying your path to point at the local venv?

marsh glade
#

yeah, it has to be something like this

polar wind
#

pyenv and.python-version or anything like that?

inland summit
# polar wind yes, and I'm not sure why that is yet, but without `$(...)` you aren't executing...

He has his reasons somewhere..
https://github.com/pdm-project/pdm/issues/2249#issuecomment-1715719491 for example, but there are more

but yeah.. it kinda sucks but i have some aliases and scripts that help me

alias av "source .venv/bin/activate.fish" # from inside a dir and venv deactivated

and my auto one https://github.com/JacobCoffee/dotfiles/blob/main/.config/fish/functions/vswp.fish which basically does

eval (pdm venv activate)

but can take a discriminator like a venv name since we have a few

inland summit
marsh glade
#

it's just the linting

#

not the pre-commit

inland summit
#

thats what linting does, do you mean the type-check?

#

if so what about manually pdm run dmypy run?

marsh glade
#

sorry, yes. Type check

polar wind
#

pre-commit most likely works fine b/c its managing its own environments for each hook

#

so pdm run dmypy run works?

marsh glade
# 
❯ pdm run dmypy run
Success: no issues found in 652 source files
inland summit
#

this makes me thing make isnt running in the right venv

polar wind
#

I'm wondering if we should try to simplify the Makefile a bit, and remove all of the venv discovery logic, making it the callers responsibility to ensure they are calling it inside the right environment

#

i.e., if you don't want to activate a venv, then run pdm run make lint

#

or if you have a venv active, just run make lint

#

etc

marsh glade
#

i actually think this issue is unrelated to this

polar wind
#

afaict, the makefile always thinks I don't have a venv because I don't have .venv/bin/activate

#

so I don't know why it works for me

marsh glade
#

I think something is strange with my python env

#

or a config somewhere else

polar wind
#

why do we need to do USING_PDM in there?

marsh glade
#

we can remove that extra check. It used to work for PDM or Poetry

polar wind
#

ahh i see, makes sense

#

still, the Makefile assumes that one is using a venv in project, called .venv - but pdm doesn't enforce either of those things

marsh glade
#

yeah, it assumes you'll install with make install

#

i'm wondering if you see the same issue if you do make install && make lint

polar wind
#

but that also changes your pdm config to use venv.in_project - which is global isn't it?

#

@if [ "$(USING_PDM)" ]; then $(PDM) config venv.in_project true && python3 -m venv --copies .venv && . $(ENV_PREFIX)/activate && $(ENV_PREFIX)/pip install --quiet -U wheel setuptools cython mypy pip; fi

#

so this line is modifying user's global pdm config, I think

marsh glade
#

yes, we can remove that line if you prefer

#

it is

inland summit
#

We can do project-wide config instead

polar wind
#

I understand why its there, from a beginner standpoint running make install is going to be a lot easier, and I can see how having some opinion involved in that scenario is good

#

yeah, that would be better if we could confine it to the project

marsh glade
#

fyi these packages wheel setuptools cython mypy pip were installed separately because in poetry i had issues with other packages not cythonizing or compiling with mypyc without it. We may no longer need it with the swap to PDM

inland summit
# inland summit We can do project-wide config instead

this is what I have

Home configuration (/Users/jcoffee5/Library/Application Support/pdm/config.toml):
venv.in_project = True
venv.with_pip = True

Project configuration (/Users/jcoffee5/git/public/litestar-org/litestar/pdm.toml):
venv.in_project = False
venv.with_pip = True
#

I don't like the extra pdm.toml, maybe it can read from pyproject

polar wind
#

I don't know if we want to set that in pyproject though b/c it would apply to all users then

inland summit
#

huh

polar wind
#

if make install could generate a pdm.toml that sets venv.in_project = True inside it, that would be better than changing the global config

marsh glade
#

we can do that. And then we can add pdm.toml to the ignore file

polar wind
#

it's actually already in .gitignore

marsh glade
#

we just need to add --local to the config command, I think

polar wind
#

oh easy

#

so back to your issues

#

when you run at the command line pdm run dmypy run it works

marsh glade
#

yep

polar wind
#

but when you run make mypy - which runs pdm run dmypy run it doesn't

marsh glade
#

pdm run pyright is the only one that fails

polar wind
#

oh its only pyright that has issues?

#

sorry

#

ok

marsh glade
#

correct. yep

polar wind
#

and at the command line, pdm run pyright fails the same way?

marsh glade
#

yep

#

--local added a pdm.toml with the following

# 
[venv]
in_project = true
polar wind
#

sweet - that's an easy change then - anyone who already has a workflow with PDM probably won't try to use make install anyway

#

what does which pyright output?

marsh glade
# 
❯ which pyright
~/Code/Starlite/litestar/.venv/bin/pyright
#

ok, we have some progress

#

i have a feeling it's related to caching. Maybe pdm caching

# 
=> Running pyright
WARNING: there is a new pyright version available (v1.1.335 -> v1.1.336).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`

/home/oracle/Code/Starlite/litestar/litestar/channels/backends/redis.py
  /home/oracle/Code/Starlite/litestar/litestar/channels/backends/redis.py:7:12 - error: Import "importlib_resources" could not be resolved (reportMissingImports)
/home/oracle/Code/Starlite/litestar/litestar/utils/helpers.py
  /home/oracle/Code/Starlite/litestar/litestar/utils/helpers.py:101:14 - error: Import "exceptiongroup" could not be resolved (reportMissingImports)
2 errors, 0 warnings, 0 informations 
=> pyright complete
#

I got this after adding the --local and re-running make install && make lint

polar wind
#

that's definitely an improvement!

#

those missing imports are interesting

#

.venv is definitely 3.8?

grand spearBOT
#

pyproject.toml line 37

"importlib-resources>=5.12.0; python_version < \"3.9\"",
marsh glade
#

no. It's 3.11

#

wtf

#

ok, i think i have some global pdm setting that's overriding

#

ok, i'm going to take a break on this one. I can work around it until i figure out exactly what's happening

polar wind
#

if you run pyenv shell 3.8.18 before make install does it help?

#

anyway that would be my only suggestion for that

#

glad we got somewhere!

marsh glade
#

yeah, this gives me a few additional things to try

marsh glade
#

so, the --local + running the above

polar wind
#

oh great!

inland summit
#

You should be able to create a 3.8 named venv and do that pdm venv activate if that’s the case

marsh glade
#

nah, i had 3.11 set to global

#

setting the local fixed it

#

at least i think that's what happened

polar wind
#

you can do pyenv local 3.8.18 so that you don't have to run pyenv shell ... every time

#

it puts the .python-version file in your directory

marsh glade
#

yeah, I see that creates the .python-version file

inland summit
#

if you don’t want to do that each time or have non 3.8 venvs you could do the pdm venv with the specifier to select each instead of setting local for whatever version you need

polar wind
#

this is what I do - I've got 2 litestar venvs, one for 3.8 and one for 3.12

marsh glade
#

but, I don't think i need to do it again now that i have the Pyenv python version file in my folder

inland summit
#

And docs require 3.11/12 I think

polar wind
polar wind
inland summit
#

Someone should make a Python packaging tool to manage all these well

#

Like a.. pdm/hatch/poetry or something

#

I hate Python packaging 😦

polar wind
#

this environment management business is a mess

#

there are workflows that work fine (for the most part)

#

but I wouldn't want to be a beginner trying to figure this stuff out

inland summit
#

I see what poetry and pdm mean on the environment stuff but it’s not easy and pdm feels especially bad at it comparatively