Skip to content

Custom executor#51

Open
barsa-net wants to merge 6 commits intodchevell:masterfrom
barsa-net:custom-executor
Open

Custom executor#51
barsa-net wants to merge 6 commits intodchevell:masterfrom
barsa-net:custom-executor

Conversation

@barsa-net
Copy link

@barsa-net barsa-net commented Jan 3, 2023

Resolves #50

Gevent-specific tests needs to be run with python -m gevent.monkey --module pytest tests/test_gevent.py

@barsa-net
Copy link
Author

A little explain for 7c42ab7.

Gevent uses a custom _FutureProxy object that implements __getattr__ that ensures some methods like .done(), .cancel(), etc. will be returned by a contained AsyncResult object.

Using object.__gettattributes__ without a fallback fails to retrieve such methods.

@dchevell dchevell marked this pull request as ready for review January 5, 2023 22:16
@dchevell
Copy link
Owner

dchevell commented Jan 5, 2023

FYI if you're seeing the test for "propagate exception callback" failing in your gevent tests:

  1. The tests are currently failing because gevent hasn't monkeypatched threading - things like concurrent.futures.wait won't work on gevent Futures without that. Add this at the top of test_gevent.py:
from gevent import monkey
monkey.patch_all()
  1. The original test I wrote, even though it appears to pass for the stdlib ThreadPoolExecutor, is fundamentally faulty - see EXECUTOR_PROPAGATE_EXCEPTIONS does not propagate exceptions #24. That's not your problem, and fixing the above should be enough for you get your tests passing so this can be merged, but just an FYI.

@barsa-net
Copy link
Author

barsa-net commented Jan 5, 2023

I originally left the pull request as draft for two reasons:

  • I planned to write some example in the documentation but I had not time to check how you are generating it
  • There are two ways to make gevent tests pass, one is what you are suggesting, the other is cherry-picking f5fb10f that slightly changes test.yaml allowing them to pass.

On the latter, maybe doing just a monkey patch is the simplest way, I'll refactor it to do so.
Sorry, the previous sentence was completely incorrect, they have to run separately or gevent.mocking mess up the rest of the tests.

As I wrote above, f5fb10f needs to be merged on master before tests can pass...

      - name: Test with pytest
        run: |
          pytest --cov=flask_executor/ --cov-report=xml --ignore=test_gevent.py
      - name: Gevent-specific test with pytest
        run: |
          python -m gevent.monkey --module pytest tests/test_gevent.py

If you prefer I can just toss gevent tests away from this pull request and open a new one that implements just that.

@barsa-net barsa-net force-pushed the custom-executor branch 3 times, most recently from f5fb10f to a34ca4d Compare January 6, 2023 12:03
@barsa-net
Copy link
Author

Since gevent-testing is not strictly needed to implement #50, I'll just leave in this pull request the tests for what the implementation truly does.

I'll try to figure out what's the best way to enable gevent monkeypatch only for some test (if there is a way) and open another PR when it's reasonably done.

@barsa-net
Copy link
Author

I feel the PR is ready to be merged, waiting for your approval @dchevell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using unpatched ThreadPoolExecutor when using gevent

2 participants