Slowdown of pytest collection in sqlalchemy in python 3.9+

[CaselIT]

• a detailed description of the bug or problem you are having
• output of pip list from the virtual environment you are using
• pytest and operating system versions
• minimal example if possible

We noticed that from python 3.9 pytest takes a significantly more time (~4x) to collect the tests for sqlalchemy.
You can run the following docker to compare. python 3.7 and 3.8 are ok (not fast not slow), python 3.9 and 3.10 are noticeably slower

docker run -ti --name py --rm python:3.7 bash -c 'git clone https://github.com/sqlalchemy/sqlalchemy.git --depth 100; pip install pytest; cd sqlalchemy; time pytest --collect-only'
docker run -ti --name py --rm python:3.8 bash -c 'git clone https://github.com/sqlalchemy/sqlalchemy.git --depth 100; pip install pytest; cd sqlalchemy; time pytest --collect-only'
docker run -ti --name py --rm python:3.9 bash -c 'git clone https://github.com/sqlalchemy/sqlalchemy.git --depth 100; pip install pytest; cd sqlalchemy; time pytest --collect-only'
docker run -ti --name py --rm python:3.10 bash -c 'git clone https://github.com/sqlalchemy/sqlalchemy.git --depth 100; pip install pytest; cd sqlalchemy; time pytest --collect-only'

the times on my pc are as floows:

#py37
real    0m27.949s
user    0m26.717s
sys     0m1.220s
#py38
real    0m27.952s
user    0m26.893s
sys     0m1.050s
# py39
real    1m45.581s
user    1m42.574s
sys     0m2.990s
# py310
real    1m46.255s
user    1m43.648s
sys     0m2.590s

cc @zzzeek

[bluetech]

Sounds like #9144. Could you try replacing pip install pytest with pip install git+https://github.com/pytest-dev/pytest.git and check the timing?

[CaselIT]

No tests are collected when installing from git, I guess we have some incompatibility with v7 on sqlalchemy side. (no error though)

[CaselIT]

Is there a changelog for v7 so I can try seeing what needs changing? The fact that no error is raised makes it hard to pinpoint a particular incompatibility

[CaselIT]

Installing from the 6.2.x branch makes no difference in the timing

[The-Compiler]

Tentatively adding this to the 7.0 project board as a reminder to myself to take a quick look at what might be going on.

[bluetech]

Looked into it, no tests are collected due to #9277. sqlalchemy does some collection magic in its pytestplugin.py which relies on isinstance(pytest.Instance) checks, something I didn't find when looking investigating for #9277.

We should either revert #9277, or, if we consider sqlalchemy to be a unique case of this, fix it there.

---

Back to the original issue, @CaselIT you can try the commit before #9277 was merged, i.e.

pip install git+https://github.com/pytest-dev/pytest.git@17b38259fde89caaf44a4e578d9f62d001a894c9

[bluetech]

Link to the sqlalchemy pytest plugin: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/testing/plugin/pytestplugin.py (search for pytest.Instance -- two hits).

[zzzeek]

we can fix the isinstance() call, whats something we can change it towards that will work in pytest 6 and 7 ? (could just be isinstance(x, (Instance, SomethingElse)) but a duck-typing type of check would be preferred.)

[zzzeek]

btw thanks for the fast help @bluetech ! I was initially a little nervous we wouldn't be able to get quick help with a less obvious issue like this.

[bluetech]

I was only able to take a quick glance at sqlalchemy's plugin code. I can definitely look at it more later, though I can maybe give you a quick answer if you can explain why the plugin checks for pytest.Instance (what's the purpose of the checks).

(Just to be clear, we will definitely not release something that will break sqlalchemy's plugin -- it only uses public API and our policy is never to break that without at least a deprecation period. The pytest.Instance change was made after analysis of actual usage but didn't catch sqlalchemy's plugin since it only looked at standalone pytest plugins).

[bluetech]

BTW, is the plugin only used by sqlalchemy's own tests, or is it meant for public use as well?

[zzzeek]

the plugin is used in a public way by other dialects that want to integrate with SQLAlchemy's test suite. So yes, it is public for third party dialects.

we are using isinstance() because we iterate through the list of pytest items in pytest_collection_modifyitems and we expand out some of the testing objects into many more tests. As we iterate through this collection, we observed that objects of type "Instance" and these were the objects we actually needed, that is, they represent actual tests. so in this hook we need a way to iterate through items and identify test methods, like "SomeTestSuite().test_foo()" (all of our tests are methods on classes).

the other place we use it is in pytest_pycollect_makeitem where we are checking for "Instance" as a means to identify test objects that we want to include in the suite, there seems to be some kind of filtering going on here.

as far as what API we need here, it just needs to be that we are able to continue seeing individual tests as objects in some way, and identify them as such. if that's going away altogether then we have bigger issues.

[zzzeek]

correction, it looks like "Instance" is how we identify the parent object (the collector?) that owns a series of tests that we want to include or exclude.

it's just part of our traversal so we just need to know how to correctly traverse.

[nicoddemus]

it's just part of our traversal so we just need to know how to correctly traverse.

What happened is that we got rid of pytest.Instance, #9277 has a detailed description of the rationale, but in summary the collection changed from this:

<Module test_file.py>
  <Class TestIt>
    <Instance ()>
      <Function test_it>

To this:

<Module test_file.py>
  <Class TestIt>
    <Function test_it>

Looking at the plugin I see usages like this:

https://github.com/sqlalchemy/sqlalchemy/blob/9c5dfddfbd405a72d33b72ed097690a6c2c88b3a/lib/sqlalchemy/testing/plugin/pytestplugin.py#L197-L202

So I think you just need to change similar checks from:

if isinstance(item.parent, pytest.Instance)

To:

if isinstance(item.parent, pytest.Class)

You want to keep pytest<7 compatibility, so something like this would be needed at the top of the file:

if pytest.version_tuple[0] >= 7:
    CollectParent = pytest.Class
else:
    CollectParent = pytest.Instance

And then:

if isinstance(item.parent, CollectParent)

Sorry I can't test this myself now.