Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setup.py: use aiohttp==3.6.2 instead of aiohttp==3.5.4 #3362

Closed
wants to merge 8 commits into from

Conversation

belikor
Copy link
Contributor

@belikor belikor commented Jul 14, 2021

As reported in issue #2769, the lbrynet daemon's RCP server doesn't respond correctly when it is compiled against Python 3.8+, only with Python 3.7.

Instead of using aiohttp==3.5.4 which was released on January 12, 2019, we use aiohttp=3.6.2 which was released October 9, 2019, and is available in Ubuntu 20.04.

By using this newer version, the RCP server seems to work correctly both with Python 3.8 and 3.9.

We could also try a newer version, like 3.7.4 as it was launched on February 25, 2021, and is available in Arch.

Copy link
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

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

This change appears to cause the tests / integration (datanetwork) integration tests to consistently fail. There must be something in 3.6.2 that is not backwards compatible with the existing SDK code. Can you investigate?

@lbry-bot lbry-bot assigned eukreign and unassigned eukreign Jul 19, 2021
@eukreign eukreign assigned belikor and unassigned eukreign Jul 19, 2021
@belikor
Copy link
Contributor Author

belikor commented Jul 19, 2021

This change appears to cause the tests / integration (datanetwork) integration tests to consistently fail. There must be something in 3.6.2 that is not backwards compatible with the existing SDK code. Can you investigate?

I'm not entirely sure how to run the integration tests in my machine, so this may take a while. I don't see any obvious errors in the logs, other than Timeouts. We could try other versions of aiohttp perhaps, 3.6.3, 3.7.4, etc.

@lyoshenka
Copy link
Member

to run integration tests, run make test-integration

@lyoshenka
Copy link
Member

@belikor are you still planning to work on this?

@belikor
Copy link
Contributor Author

belikor commented Aug 9, 2021

@belikor are you still planning to work on this?

The integration tests pass okay in my machine (tox), so at the moment I'm not sure what to test further.

@lyoshenka
Copy link
Member

@eukreign whats the next step?

@eukreign
Copy link
Member

eukreign commented Sep 7, 2021

@lyoshenka next step would be to rebase and see if the tests pass, if they don't pass @belikor needs to figure out why they are failing

alternately, there is another PR to simply upgrade the SDK to newer Python altogether (#3404 ) but that PR is also failing (due to DHT tests not working with newer Python version)

@eukreign eukreign removed their assignment Sep 7, 2021
Copy link
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

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

tests need to pass before this can be merged

@belikor
Copy link
Contributor Author

belikor commented Sep 12, 2021

This is a very small change, which just changes aiohttp from 3.5.4 to 3.6.2. The only test that seems to fail is test_download_torrent.

python -m unittest tests.integration.datanetwork.test_file_commands.FileCommands.test_download_torrent

This test is actually pretty short, maybe some of you can figure out why it fails because the error message doesn't give much information of where it fails. The failure seems to occur in the async calls, so maybe not in the LBRY code itself.

Traceback (most recent call last):
  File "/home/runner/work/lbry-sdk/lbry-sdk/.tox/datanetwork/lib/python3.7/site-packages/lbry/testcase.py", line 143, in run
    self.loop.run_until_complete(maybe_coroutine)
  File "/opt/hostedtoolcache/Python/3.7.11/x64/lib/python3.7/asyncio/base_events.py", line 587, in run_until_complete
    return future.result()
concurrent.futures._base.CancelledError

What I notice is that this test is optional; it only runs if libtorrent is installed. As far as I can tell, LBRY provides their own copy of libtorrent (lbry-libtorrent). Maybe we need to update this code as well because it seems it was last updated early 2020. Maybe a newer libtorrent will play better with a newer aiohttp.

@coveralls
Copy link

coveralls commented Sep 12, 2021

Coverage Status

Coverage decreased (-0.05%) to 67.935% when pulling 3e01ffc on belikor:aiohttp-new into 561566e on lbryio:master.

@belikor belikor force-pushed the aiohttp-new branch 7 times, most recently from 4fdb873 to 8f7aa35 Compare September 13, 2021 22:01
@belikor belikor force-pushed the aiohttp-new branch 5 times, most recently from 407173b to 31abf2f Compare September 19, 2021 15:57
As reported in issue lbryio#2769, the `lbrynet` daemon doesn't respond
correctly when it is compiled against Python 3.8+, only with
Python 3.7.

Instead of using `aiohttp==3.5.4` which was released on
January 12, 2019, we use `aiohttp=3.6.2` which was released
October 9, 2019, and is available in Ubuntu 20.04.

By using this newer version, the RCP server seems to work
correctly both with Python 3.8 and 3.9.

We could also try a newer version, like `3.7.4` as
it was launched on February 25, 2021, and is available
in Arch.
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.

None yet

5 participants