add slotscheck to CI

[ariebovenberg]

Description

As discussed in #7589, slotscheck can prevent slots-related mistakes from creeping back in.

Note that because slotscheck imports files, additional_dependencies was needed in the pre-commit config. The versions of these dependencies are not kept in sync with sqlalchemy requirements. If this is not OK, there are alternative setups or the check could be moved to tox.

Checklist

• A short code fix
  • please include the issue number, and create an issue if none exists, which
    must include a complete example of the issue. one line code fixes without an
    issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.

Have a nice day!

[jvanasco]

Is this ready for additional testing in gerrit?

[ariebovenberg]

@zzzeek yes. I'm wondering though if pre-commit is the best place for slotscheck. Is there a specific reason some checks are in tox/pre-commit/github-actions?

[zzzeek]

@zzzeek yes. I'm wondering though if pre-commit is the best place for slotscheck. Is there a specific reason some checks are in tox/pre-commit/github-actions?

Things in tox are tested in Jenkins CI. Things in github actions are tested there. things in pre-commit are run every time we commit.

so in pre-commit, black and zimports are there since they actually change the code. so those only belong in pre-commit. but we also have flake8 there, because it catches so many quick things very fast, it just helps filter out quick issues before we push up to gerrit, which would then kick of jenkins builds which are more time / resource consuming.

my impression of slotscheck is that it can work as a quick flake8- sort of thing. but yes, overall it should likely be part of tox and github actions as well. it seems right now to not fit cleanly into any of our particular targets, but it's more convenient for us if we make it part of the existing targets - the pep8 target is right now where we have generic "code quality" checks where we not only call upon flake8 (which checks way more than pep8) we also have "black check" in there.

id have to try the tool out a little bit to get a feel for it but yes I think it would first be part of the "pep8" target at the moment, how does that seem?

[ariebovenberg]

@zzzeek thanks for the comprehensive answer! I initially added it to pre-commit following the suggestion in #7589 (comment), but I'd say slotscheck fits more in the tox/GH-actions crowd than pre-commit.

There was a similar discussion in aio-libs/aiohttp#6547 (comment), where the conclusion was to add slotscheck to GH actions, not pre-commit. The reason was that slots are not frequently changed, reducing the benefits of running the check every commit.

[zzzeek]

that's fine let's do it that way then!

[ariebovenberg]

• I've moved slotscheck to tox.ini
• I took the liberty of renaming pep8 to lint, as this name better reflects the diversity of checks included. Even black is technically not fully PEP8 compliant

[zzzeek]

the rename is fine but you would need to add another target pep8 that imports the "lint" target, so that our CI which is hardcoded to "pep8" for all projects continues to work, until we can make this name change for all projects (alembic, dogpile, mako)

[ariebovenberg]

I've re-added pep8 env which takes its configuration from the lint env

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 3e77fe5 of this pull request into gerrit so we can run tests and reviews and stuff

[zzzeek]

lets try it out

[sqla-tester]

New Gerrit review created for change 3e77fe5: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

mike bayer (zzzeek) wrote:

just gave it a spin removing a "slots()", to see how straightforward it is.

output is very clear:

ERROR: 'sqlalchemy.sql.roles:DMLTableRole' has slots but superclass does not.
ERROR: 'sqlalchemy.sql.roles:StrictFromClauseRole' has slots but superclass does not.
Oh no, found some problems!
Scanned 207 module(s), 1347 class(es).

very nice!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

mike bayer (zzzeek) wrote:

I've searched the entire codebase of slotscheck to see where it's using mypy, and I see nothing; no imports of anything in mypy, no command line running of mypy. Why is mypy a dependency?

https://github.com/ariebovenberg/slotscheck/search?q=mypy

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

mike bayer (zzzeek) wrote:

oh that's to import our mypy plugin. OK. let me alter that to indicate it's using the sqlalchemy[mypy] target. I found some other slots checks that were missing because they are in our non-C ext fallback code, so I am fixing that also.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

Federico Caselli (CaselIT) wrote:

Looks ok to me

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[ariebovenberg]

@zzzeek indeed mypy is required to import sqlalchemy's mypy plugin.

Regarding C-extensions, the situation here is interesting! It looks like it may be needed to have two slotscheck checks (one HAS_CYEXTENSION, the other not). Looking at sqlalchemy.util/_collections.py I can see:

if typing.TYPE_CHECKING or not HAS_CYEXTENSION:
    from ._py_collections import ImmutableContainer
    # ...
else:
    from sqlalchemy.cyextension.immutabledict import ImmutableContainer
    # ...

where both ImmutableContainers were missing __slots__ 🤔.
In PR #7589 I fixed cyextension/immutabledict.pyx:ImmutableContainer, but not the pure Python one.

[zzzeek]

@zzzeek indeed mypy is required to import sqlalchemy's mypy plugin.

Regarding C-extensions, the situation here is interesting! It looks like it may be needed to have two slotscheck checks (one HAS_CYEXTENSION, the other not). Looking at sqlalchemy.util/_collections.py I can see:

if typing.TYPE_CHECKING or not HAS_CYEXTENSION:
    from ._py_collections import ImmutableContainer
    # ...
else:
    from sqlalchemy.cyextension.immutabledict import ImmutableContainer
    # ...

where both ImmutableContainers were missing __slots__ thinking. In PR #7589 I fixed cyextension/immutabledict.pyx:ImmutableContainer, but not the pure Python one.

oh i wasnt even sure if cython was an option here (@CaselIT does slots apply to cython even? I thought we have to name all the variables anyway?) . OK, well, if cython builds, the py versions of the objects are still available, and slotscheck is probably checking them. in this case it likely missed these two because there were no inheriting objects from them at runtime.

[CaselIT]

oh i wasnt even sure if cython was an option here (@CaselIT does slots apply to cython even? I thought we have to name all the variables anyway?)

that's on cdef classes. a plain class in cython, like ImmutableContainer behaves the same as a python one

[sqla-tester]

mike bayer (zzzeek) wrote:

oh i know how to fix this

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[ariebovenberg]

OK, well, if cython builds, the py versions of the objects are still available, and slotscheck is probably checking them

slotscheck does recurse properly into extension modules. Because it's a 'runtime' check it is also able to follow conditional imports.

But of course, it cannot trace multiple code branches at once (HAS_CYEXTENSION and not HAS_CYEXTENSION). For full slotscheck 'coverage' you'd have to run on both types of installations.

This 'double' check is the only way to find both ImmutableContainer problems. If HAS_CYEXTENSION thenImmutableProperties inherits from cyextension.pyx:ImmutableContainer, otherwise ImmutableProperties inherits from _py_collections.py:ImmutableContainer. Both base classes need __slots__ because ImmutableProperties does, but never "at the same time"

[zzzeek]

yup, all makes sense, thanks

[sqla-tester]

mike bayer (zzzeek) wrote:

ah, we only run lint on ubuntu, we're good

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

mike bayer (zzzeek) wrote:

the lint action doesn't, problem solved

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3576 has been merged. Congratulations! :)