Skip to content

Update to Python 3.10 and async-rediscache v1.0.0#2229

Merged
ChrisLovering merged 20 commits into
mainfrom
py3.10-rediscache
Aug 14, 2022
Merged

Update to Python 3.10 and async-rediscache v1.0.0#2229
ChrisLovering merged 20 commits into
mainfrom
py3.10-rediscache

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering commented Jul 23, 2022

Description

This PR does the following things related to the PR description:

  1. Bumps the Python version up to 3.10, ensuring CI is updated too.
  2. Bumps the version of bot-core used to v8.0.0-beta.3 and pins transitive deps to exact versions
  3. Fixed breaking changes from the aioredis -> redis-py migration in async-rediscache v1.0.0-rc2

This PR also has the following kaizen commits:

  1. Mark the correct snekbox service as a dependency of the bot service
  2. Remove the, deprecated as of 3.10, call to get_event_loop() when there is no running loop
  3. Remove warnings in error handler tests
  4. Bump all dependencies to latest

@python-discord-policy-bot python-discord-policy-bot Bot requested a review from a team July 23, 2022 22:08
@ChrisLovering ChrisLovering added a: dependencies Related to package dependencies and management p: 1 - high High Priority t: enhancement Changes or improvements to existing features s: needs review Author is waiting for someone to review and approve review: do not merge The PR can be reviewed but cannot be merged now labels Jul 23, 2022
@ChrisLovering ChrisLovering force-pushed the py3.10-rediscache branch 3 times, most recently from 58c858f to 7d924d4 Compare July 25, 2022 22:34
Copy link
Copy Markdown
Contributor

@HassanAbouelela HassanAbouelela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a well polished PR. I like the test changes, and I didn't find any issues while testing. Signing off on the docker changes on behalf of dev-ops.

@ChrisLovering ChrisLovering force-pushed the py3.10-rediscache branch 2 times, most recently from 17251de to a6770ce Compare July 27, 2022 20:46
global redis_session
redis_session = RedisSession(use_fakeredis=True)
redis_loop.run_until_complete(redis_session.connect())
asyncio.run(redis_session.connect())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong because asyncio.run creates a new event loop. Presumably this will be separate from the event loop the rest of the tests use, unless unittest accounts for that somehow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a running loop when redis_loop was created, nor is there one during module setup, so the asyncio.get_event_loop() call was raising a deprecation error.

I think the alternative to this is to create the session within each test class as an asyncSetUp, rather than at the module level.

Copy link
Copy Markdown
Member Author

@ChrisLovering ChrisLovering Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed both 1b6b76f (#2229) and b5797ff (#2229) to show what this alternative could look like.

@ChrisLovering ChrisLovering removed the review: do not merge The PR can be reviewed but cannot be merged now label Jul 31, 2022
@ChrisLovering
Copy link
Copy Markdown
Member Author

ChrisLovering commented Aug 3, 2022

I don't understand the warnings fix here. setup is a coroutine function rather than a coroutine itself, so there should be no warnings from it just being imported, only if it is called and the coroutine returned not awaited (which doesn't seem to happen).

By importing the coro directly, the below warnings are raised (malloc enabled to see object allocation)

tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_already_handled
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_check_failure
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_invoke_error
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_not_found_error_invoked_by_handler
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_not_found_error_not_invoked_by_handler
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_on_cooldown
  C:\Users\chris\src\bot\.venv\lib\site-packages\_pytest\python.py:776: RuntimeWarning: coroutine 'setup' was never awaited
    func(arg)

  Object allocated at:
    File "C:\Users\chris\src\bot\.venv\lib\site-packages\execnet\gateway_base.py", line 1237
      self.stack.append(as_bytes.decode("utf-8"))

I am not too sure why this happens either, as the coro is never called, but by namespacing the import like this the warnings go away.

Changing the cog to be shared across different tests in ErrorHandlerTests doesn't seem ideal, as some of the tests patch methods meaning they could affect each other. I cant see any obvious places it would actually make a difference here though,

This is fine with unnittest as the setUp function gets called before each test method.

@wookie184
Copy link
Copy Markdown
Contributor

I don't understand the warnings fix here. setup is a coroutine function rather than a coroutine itself, so there should be no warnings from it just being imported, only if it is called and the coroutine returned not awaited (which doesn't seem to happen).

By importing the coro directly, the below warnings are raised (malloc enabled to see object allocation)

tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_already_handled
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_check_failure
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_invoke_error
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_not_found_error_invoked_by_handler
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_not_found_error_not_invoked_by_handler
tests/bot/exts/backend/test_error_handler.py::ErrorHandlerTests::test_error_handler_command_on_cooldown
  C:\Users\chris\src\bot\.venv\lib\site-packages\_pytest\python.py:776: RuntimeWarning: coroutine 'setup' was never awaited
    func(arg)

  Object allocated at:
    File "C:\Users\chris\src\bot\.venv\lib\site-packages\execnet\gateway_base.py", line 1237
      self.stack.append(as_bytes.decode("utf-8"))

I am not too sure why this happens either, as the coro is never called, but by namespacing the import like this the warnings go away.

I had a look into it and it seems to be fundamentally because of this: https://nose.readthedocs.io/en/latest/doc_tests/test_doctest_fixtures/doctest_fixtures.html?highlight=setup#module-level-fixtures

pytest sees a function called setup and tries to run it as a setup function for the tests, but doesn't await it hence the error.

The check for setup seems to be behind a check for the nose plugin in pytest (https://github.com/pytest-dev/pytest/blob/f43ddd8acd74a2c3242a3874420072836e0614aa/src/_pytest/python.py#L881), so the only thing I'm not sure about is why that would trigger since we don't use nose.

Anyway, maybe we should add a comment saying something like "setup should not be imported directly or pytest will try and run it" just to be clear?

Changing the cog to be shared across different tests in ErrorHandlerTests doesn't seem ideal, as some of the tests patch methods meaning they could affect each other. I cant see any obvious places it would actually make a difference here though,

This is fine with unnittest as the setUp function gets called before each test method.

Ooh yeah of course.

@HassanAbouelela
Copy link
Copy Markdown
Contributor

The check for setup seems to be behind a check for the nose plugin in pytest (https://github.com/pytest-dev/pytest/blob/f43ddd8acd74a2c3242a3874420072836e0614aa/src/_pytest/python.py#L881), so the only thing I'm not sure about is why that would trigger since we don't use nose.

You aren't quite looking at the right line:
https://github.com/pytest-dev/pytest/blob/7.1.x/src/_pytest/python.py#L561-L568

This doesn't use nose, and it's what's called for us. Refer to TB:

    File "site-packages\_pytest\fixtures.py", line 1111
      result = call_fixture_func(fixturefunc, request, kwargs)
    File "site-packages\_pytest\fixtures.py", line 883
      fixture_result = next(generator)
    File "site-packages\_pytest\python.py", line 563
      _call_with_optional_argument(setup_module, request.module)
    File "site-packages\_pytest\python.py", line 776
      func(arg)

Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing, other than that all seems good, nice work 💯

Comment thread pyproject.toml Outdated
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks ❤️

@ChrisLovering ChrisLovering requested a review from MarkKoz August 4, 2022 14:06
The bot service was still configured to depend on the snekbox service, even though this service is now optional, in favour of the snekbox-311 service.
get_event_loop is deprecated as of 3.10 if there is no running loop.
These warnings were caused by the setup coro from error_handler.py being imported directly, causing a warning about an un-awaited coro whenever the Cog was accessed from the same file.
This bump comes with a move to redis-py over aioredis. As such, pin new transitive dependancies to exact versions.
This commit resolves all the breaking changes from the aioredis -> redis-py migration.
This has been abstracted away, the correct way to do this now is to directly access the client.
If a float is given, Redis will assume the expiry is in milliseconds and must be multiplied by 1000. This is undesirable, as we are already passing the expiry in seconds.
This was a new lint rule added in the latest bugbear.
pep-naming now supports these functions being in camel case.
This helper ensures that a fresh RedisSession is given to each test case that inherits from it.
The new versions introduce conversions which causes the doc command
embed to be formatted improperly
@ChrisLovering ChrisLovering merged commit 02cff85 into main Aug 14, 2022
@ChrisLovering ChrisLovering deleted the py3.10-rediscache branch August 14, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: dependencies Related to package dependencies and management p: 1 - high High Priority s: needs review Author is waiting for someone to review and approve t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants