[TINKERPOP-3116] async_timeout not declared in gremlinpython dependencies

[JonZeolla]

This adds async_timeout to gremlin-python's setup.py as a runtime dependency which is required when using python >= 3.11

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.42%. Comparing base (d9e34fb) to head (707ec70).
Report is 136 commits behind head on 3.6-dev.

Additional details and impacted files

@@              Coverage Diff              @@
##             3.6-dev    #2844      +/-   ##
=============================================
+ Coverage      75.14%   75.42%   +0.27%     
- Complexity     12346    12371      +25     
=============================================
  Files           1058     1033      -25     
  Lines          63610    59912    -3698     
  Branches        6962     6976      +14     
=============================================
- Hits           47801    45189    -2612     
+ Misses         13225    12328     -897     
+ Partials        2584     2395     -189     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kenhuuu]

Hi and thanks for contributing. Just to let you know, we are currently in code freeze.

Could you explain a bit more about why this is needed? If I run something like pip install gremlinpython==3.6.7, I see that async_timeout is installed as a transitive dependency.

Collecting async-timeout<5.0,>=4.0
  Downloading async_timeout-4.0.3-py3-none-any.whl

Edit: just realized that you made a Jira for this that included more information. I can't quite repro what you saw using Fedora-based distro and Python 9. Were you just trying to run the examples?

[JonZeolla]

I was confirming connectivity from my host to the gremlin-server docker image as I was putting together a local testing environment.

Yeah that makes sense that this used to be solved via a transitive dependency; that was my gut feeling as well. I'll dig more into this and report back on the specifics, however I suggest that this be an explicit direct dependency either way because it's referenced in the first party code.

[JonZeolla]

It seems to be related to the version of python; when using python >= 3.11, aiohttp v3.10.10 does not include async_timeout as a transitive dependency.

3.10:

➜ uv add 'gremlinpython; python_version == "3.10"'
Using CPython 3.10.15
Removed virtual environment at: .venv
Creating virtual environment at: .venv
Resolved 17 packages in 9ms
Installed 16 packages in 15ms
 + aenum==3.1.15
 + aiohappyeyeballs==2.4.3
 + aiohttp==3.10.10
 + aiosignal==1.3.1
 + async-timeout==4.0.3
 + attrs==24.2.0
 + frozenlist==1.5.0
 + gremlinpython==3.7.2
 + idna==3.10
 + isodate==0.7.2
 + multidict==6.1.0
 + nest-asyncio==1.6.0
 + propcache==0.2.0
 + six==1.16.0
 + typing-extensions==4.12.2
 + yarl==1.16.0
➜ uv tree
Resolved 17 packages in 5ms
example v0.1.0
└── gremlinpython v3.7.2
    ├── aenum v3.1.15
    ├── aiohttp v3.10.10
    │   ├── aiohappyeyeballs v2.4.3
    │   ├── aiosignal v1.3.1
    │   │   └── frozenlist v1.5.0
    │   ├── async-timeout v4.0.3
    │   ├── attrs v24.2.0
    │   ├── frozenlist v1.5.0
    │   ├── multidict v6.1.0
    │   │   └── typing-extensions v4.12.2
    │   └── yarl v1.16.0
    │       ├── idna v3.10
    │       ├── multidict v6.1.0 (*)
    │       └── propcache v0.2.0
    ├── isodate v0.7.2
    ├── nest-asyncio v1.6.0
    └── six v1.16.0
(*) Package tree already displayed

3.11:

➜  uv add 'gremlinpython; python_version == "3.11"'
Using CPython 3.11.10
Removed virtual environment at: .venv
Creating virtual environment at: .venv
Resolved 15 packages in 5ms
Prepared 5 packages in 246ms
Installed 14 packages in 20ms
 + aenum==3.1.15
 + aiohappyeyeballs==2.4.3
 + aiohttp==3.10.10
 + aiosignal==1.3.1
 + attrs==24.2.0
 + frozenlist==1.5.0
 + gremlinpython==3.7.2
 + idna==3.10
 + isodate==0.7.2
 + multidict==6.1.0
 + nest-asyncio==1.6.0
 + propcache==0.2.0
 + six==1.16.0
 + yarl==1.16.0
➜ uv tree
Resolved 15 packages in 8ms
example v0.1.0
└── gremlinpython v3.7.2
    ├── aenum v3.1.15
    ├── aiohttp v3.10.10
    │   ├── aiohappyeyeballs v2.4.3
    │   ├── aiosignal v1.3.1
    │   │   └── frozenlist v1.5.0
    │   ├── attrs v24.2.0
    │   ├── frozenlist v1.5.0
    │   ├── multidict v6.1.0
    │   └── yarl v1.16.0
    │       ├── idna v3.10
    │       ├── multidict v6.1.0
    │       └── propcache v0.2.0
    ├── isodate v0.7.2
    ├── nest-asyncio v1.6.0
    └── six v1.16.0

[kenhuuu]

Thanks for taking the time to investigate this.

however I suggest that this be an explicit direct dependency either way because it's referenced in the first party code

Agreed.

VOTE +1

[Cole-Greer]

Hi @JonZeolla, thanks for opening this PR, it makes sense that we should directly declare this dependency. I just checked the dependency tree from gremlinpython 3.6.7 in my environment, and I'm seeing that async_timeout is transitively included with a looser version restriction than what you are explicitly adding >=4.0.0a3,<5.0. Would you be ok with loosening the version range in this PR such that we aren't increasing any restrictions on users with this change?

gremlinpython==3.6.7
├── aenum [required: >=1.4.5,<4.0.0, installed: 3.1.15]
├── aiohttp [required: >=3.8.0,<4.0.0, installed: 3.8.6]
│   ├── aiosignal [required: >=1.1.2, installed: 1.3.1]
│   │   └── frozenlist [required: >=1.1.0, installed: 1.4.0]
│   ├── async-timeout [required: >=4.0.0a3,<5.0, installed: 4.0.3]
│   ├── attrs [required: >=17.3.0, installed: 23.1.0]
│   ├── charset-normalizer [required: >=2.0,<4.0, installed: 3.3.0]
│   ├── frozenlist [required: >=1.1.1, installed: 1.4.0]
│   ├── multidict [required: >=4.5,<7.0, installed: 6.0.4]
│   └── yarl [required: >=1.0,<2.0, installed: 1.9.2]
│       ├── idna [required: >=2.0, installed: 3.4]
│       └── multidict [required: >=4.0, installed: 6.0.4]

[JonZeolla]

@Cole-Greer I looked at that a bit, but versions prior to 4.0.3 don't support Python 3.11 according to their changelog, so I thought this would be more appropriate given:

tinkerpop/gremlin-python/src/main/python/setup.py

Line 90 in 8d3117e

python_requires='>=3.9'

Perhaps a middle ground is something like >=4.0.0a3,<5.0 for the 3.6-dev branch and then something more aggressive like >=4.0.3,<5.0 for the 3.7-dev branch?

Speaking of bigger changes, using async-timeout directly is officially deprecated for newer versions of python:

This library has effectively been upstreamed into Python 3.11+. Therefore this library is considered deprecated and no longer supported. We'll keep the project open in the unlikely case of security issues until Python 3.10 is officially unsupported.

So perhaps in the 3.7-dev branch I could remove it entirely for python >= 3.11 like they suggest:

if sys.version_info >= (3, 11):
    import asyncio as async_timeout
else:
    import async_timeout

and update the dependency to be something like:

async-timeout >= 4; python_version < "3.11"

If we're on the same page I'd be happy to open a separate PR for 3.7-dev after a 3.6 -> 3.7 sync to either use >=4.0.3,<5.0 or remove the dependency altogether for python >= 3.11

Side note: 4.0.0 and 4.0.0a3 are slightly different and typically I wouldn't suggest allowing an alpha release but it seems that decision was really made upstream in aiohttp so I'm OK with aligning to that.

[Cole-Greer]

Thanks @JonZeolla, both 3.6-dev and 3.7-dev are accumulating changes for minor version releases, and in that regard they are equivalent in their lack of tolerance for breaking changes. Breaking changes should target our master branch which will be picked up in our next major release.

That being said, looking at their changelog, it seems very unlikely that someone would be using say 4.0.1 and unable to upgrade to 4.0.3. I was against imposing a slight restriction here without justification, although I think in this case the ability to support python 3.11 far outweighs the slight restriction of >=4.0.3,<5.0. I support keeping this PR as is, and I can merge it through 3.6-dev, 3.7-dev, and master.

I think a followup change to remove async_timeout for python versions >= 3.11 as you suggested makes sense for the master branch.

VOTE +1

[xiazcy]

Thank you all for the thoughtful discussion.

VOTE +1