Add async networking libraries support #210

Albo90 · 2022-03-09T13:39:46Z

Part of feature: Add async networking libraries support #208
intended to be merged to feature-branch only at the moment

feat: add support for async http library aiohttp

Async python no duplicate code

cla-assistant · 2022-03-09T13:40:08Z

All committers have signed the CLA.

phanak-sap · 2022-03-09T15:37:57Z

I did checkout your code and try to run the following easy one async request example, modified from the generic sample from the issue record:

import asyncio
import aiohttp
from pyodata.client import Client

async def main():
    SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/'

    async with aiohttp.ClientSession() as client:
      northwind = await Client.build_async_client(SERVICE_URL , client)
      order = northwind.entity_sets.Order_Details.get_entity(OrderID=10248, ProductID=42).execute()
      print(order.Quantity)
      
async_entities = asyncio.run(main())

Got following error:

  File "..\pyodata\v2\service.py", line 354, in _call_handler
    self._logger.debug('  url: %s', response.url)
AttributeError: '_RequestContextManager' object has no attribute 'url'

which is from this PR code snippet

  def _call_handler(self, response):
       self._logger.debug('Received response')
       self._logger.debug('  url: %s', response.url)

Albo90 · 2022-03-09T18:12:07Z

Syntax is different. I posted an example into issue yesterday, and I explained like syntax is different from sync version. In my opinion the main change is the getattr use because we can't transform getattribute as async without huge and maybe dangerous impact.

Your example with correct syntax:

import asyncio

import aiohttp
from pyodata.client import Client


async def main():
    SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/'

    async with aiohttp.ClientSession() as client:
        northwind = await Client.build_async_client(SERVICE_URL, client)
        order = await northwind.entity_sets.Order_Details.get_entity(OrderID=10248, ProductID=42).async_execute()
        print(await order.getattr('Quantity'))


async_entities = asyncio.run(main())

phanak-sap · 2022-03-09T18:36:42Z

Oh, I see my error, I forgot to change the execute() to async_execute(). Of course - knew about it and still.. :)
Works OK.

Wonder if we could somehow distinguish correctly what kind of client instance was passed and perhaps throw an exception if execute() (and other methods intended for standard usage) is called with async client and vice-versa. If it is just the difference between pyodata.Client and pyodata.Client() constructor call versus pyodata.Client.build_async_client() then it should be easy.

phanak-sap · 2022-03-09T19:22:47Z

pyodata/v2/response.py

+
+
+class Response:
+    """Representation of http response in a standard form already used by handlers"""


Odata v2 has both JSON and XML Atom as standard response...
https://www.odata.org/documentation/odata-version-2-0/atom-format/

This stuff is already handled in ODataHttpResponse class. Am I missing something why this module needs to be created as well?

phanak-sap · 2022-03-09T19:28:41Z

pyodata/v2/service.py

@@ -858,6 +889,19 @@ def __getattr__(self, attr):
                raise AttributeError('EntityType {0} does not have Property {1}: {2}'
                                     .format(self._entity_type.name, attr, str(ex)))

+    async def getattr(self, attr):


I guess this method should have async_ prefix as well, similar to other new ones.

Will throw RuntimeError when called in synchronous script, where it looks like correct usage until seen the pyodata source code I guess.

import requests import pyodata import logging logging.basicConfig(level=logging.DEBUG) SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/' northwind = pyodata.Client(SERVICE_URL, requests.Session()) order = northwind.entity_sets.Order_Details.get_entity(OrderID=10248, ProductID=42).execute() print(order.getattr('Quantity'))

<coroutine object EntityProxy.getattr at 0x000002032DFD7CA0> ...: RuntimeWarning: coroutine 'EntityProxy.getattr' was never awaited print(order.getattr('Quantity'))

phanak-sap · 2022-03-09T20:16:15Z

@Albo90 Totally lame question (async python knowledge) - could you please provide sample script where the new async .getattr should be used as intended; compared to getting the value old vay trough non-async __getattr__? I guess passing to another async function (not just logging or printing).

You can modify your example hardcoded against Nortwind, with both getters being used:

import asyncio
import aiohttp
from pyodata.client import Client

import logging
logging.basicConfig(level=logging.DEBUG)

import asyncio
async def main():
    async with aiohttp.ClientSession() as client:
        logger = logging.getLogger("SCRIPT")
        SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/'
        service = await Client.build_async_client(SERVICE_URL, client)
        entities = await service.entity_sets.Orders.get_entities().async_execute()

        for entity in entities:
            attr_value = await entity.getattr("OrderID")            
            logger.info('Order ID value (version: %d)', attr_value)
            print(attr_value)
            print(entity.OrderID,entity.EmployeeID,entity.CustomerID)
        return entities

async_entities = asyncio.run(main())

Albo90 · 2022-03-09T22:47:28Z

@Albo90 Totally lame question (async python knowledge) - could you please provide sample script where the new async .getattr should be used as intended; compared to getting the value old vay trough non-async __getattr__? I guess passing to another async function (not just logging or printing).

You can modify your example hardcoded against Nortwind, with both getters being used:
import asyncio
import aiohttp
from pyodata.client import Client

import logging
logging.basicConfig(level=logging.DEBUG)

import asyncio
async def main():
    async with aiohttp.ClientSession() as client:
        logger = logging.getLogger("SCRIPT")
        SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/'
        service = await Client.build_async_client(SERVICE_URL, client)
        entities = await service.entity_sets.Orders.get_entities().async_execute()

        for entity in entities:
            attr_value = await entity.getattr("OrderID")            
            logger.info('Order ID value (version: %d)', attr_value)
            print(attr_value)
            print(entity.OrderID,entity.EmployeeID,entity.CustomerID)
        return entities

async_entities = asyncio.run(main())

I answer you about async and coroutine, tomorrow I send you more details for each point. For me the most important information about async is to understand that an async method cannot be used like sync otherwise the result is "RuntimeWarning: coroutine". For that reason getattr is only for async and we didn't use your getattribute, it's a python standard method and it's sync, if we changed it with async (I think it's impossible but I use it to explain you) you standard code doesn't work .

phanak-sap · 2022-03-10T11:37:05Z

Yeah I run into "RuntimeWarning: coroutine" when playing with your code and I understand why is that. I will be happy for any answer of course, but it is valid "just read PEP 492 and asyncio/aiohttp documentation" also :)

So far it seems the new stuff from this PR is not affecting the old, so no backward incompatible changes for standard, synchronous usage of pyodata. That's is the main focus for the feature in fact, no breaking of existing code out there. Pyodata should remain working correctly with synchronous and standard multithreading usage.

I asked basically for a simplest snippet possible for documentation, I know that little bit early on.

Albo90 · 2022-03-10T12:13:25Z

In the evening we will add these feature:

Async prefix to method getattr.
Atom format for the response
Documentation and test (at least a part of those)

For the async argument, I attach an example:

import asyncio

import aiohttp
from pyodata.client import Client


async def main():
    SERVICE_URL = 'http://services.odata.org/V2/Northwind/Northwind.svc/'

    async with aiohttp.ClientSession() as client:
        northwind = await Client.build_async_client(SERVICE_URL, client)
        orders = await northwind.entity_sets.Order_Details.get_entities().select('OrderID,ProductID').async_execute()
        for order in orders:
            # quantity = await order.getattr('Quantity')
            quantity = order.Quantity
            print(quantity)


async_entities = asyncio.run(main())

An async method like getattr need "await" when it is called. If you don't set it, you create only coroutine. The coroutine cannot be print or interrogate, it can be only executed with await. The problem is your implementantion of __ getattribute __ uses cache to permit for example "order.Quantity" when you use async it could have a problem so we defined getattr, if you want I can make an example where "order.Quantity" doesn't work but "await order.getattr("Quantity")" works.

phanak-sap · 2022-03-11T09:44:47Z

Hi @Albo90

I am sorry, we have not yet enabled Github Actions for this feature branch as well. At the moment they run against master PRs only. But all your new tests are failing (my laptop as a CI server :) ).

You are perhaps using libraries that are part of your system PIP installed packages, but missing that in the PR code.
(update dev-requirements.txt file accordingly)

Please use following (testing) workflow for your local work:

use virtualenv, so you are isolated from your system-level installed packages
activate the virtualenv according your OS
pip install -r dev-requirements.txt
pip install -r requirements.txt
pytest --cov-report term --cov=pyodata
pylint --rcfile=.pylintrc --output-format=parseable --reports=no pyodata
flake8 --config=.flake8 pyodata

There is a Makefile with all targets that can be used as well, if you are fan of makefiles.

Errors I see with the checkout of currently latest commit a4cc84d:

PytestUnhandledCoroutineWarning: async def functions are not natively supported and have been skipped.
  You need to install a suitable plugin for your async framework, for example:
    - anyio
    - pytest-asyncio
    - pytest-tornasync
    - pytest-trio
    - pytest-twisted
    warnings.warn(PytestUnhandledCoroutineWarning(msg.format(nodeid)))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===================================== 242 passed, 8 skipped, 14 warnings in 4.46s =====================================

Even if I install pytest-asyncio plugin as a naive solution, the tests are still failing:


________________________________ ERROR at teardown of test_client_custom_configuration ________________________________

    @pytest.fixture
    def event_loop():
        loop = asyncio.new_event_loop()
        yield loop
>       loop.run_until_complete(asyncio.sleep(0.1, loop=loop))
E       TypeError: sleep() got an unexpected keyword argument 'loop'

=============================================== short test summary info ===============================================
ERROR tests/test_async_client.py::test_invalid_odata_version - TypeError: sleep() got an unexpected keyword argument ...
ERROR tests/test_async_client.py::test_create_client_for_local_metadata - TypeError: sleep() got an unexpected keywor...
ERROR tests/test_async_client.py::test_create_service_application[application/xml] - TypeError: sleep() got an unexpe...
ERROR tests/test_async_client.py::test_create_service_application[application/atom+xml] - TypeError: sleep() got an u...
ERROR tests/test_async_client.py::test_create_service_application[text/xml] - TypeError: sleep() got an unexpected ke...
ERROR tests/test_async_client.py::test_metadata_not_reachable - TypeError: sleep() got an unexpected keyword argument...
ERROR tests/test_async_client.py::test_metadata_saml_not_authorized - TypeError: sleep() got an unexpected keyword ar...
ERROR tests/test_async_client.py::test_client_custom_configuration - TypeError: sleep() got an unexpected keyword arg...
====================================== 250 passed, 1 warning, 8 errors in 2.52s =======================================

phanak-sap · 2022-03-11T11:15:31Z

@Albo90 please agree to the CLA - see https://github.com/SAP/python-pyodata/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco

phanak-sap · 2022-03-11T17:18:38Z

dev-requirements.txt

@@ -1,4 +1,6 @@
 requests==2.23.0
+pytest-asyncio == 0.15.1
+aiohttp==3.8.1


I have already updated as first commit to the async-feature branch the dev-dependency.txt pytest-aiohttp>=1.0.4.

Wouldn't explicit pytest plugin by better than generic aiohttp library? Just asking what dependency is better, doesn't seem right to have both.

Still have same 8 errors running pytest TypeError: sleep() got an unexpected keyword even with the newly added dev-requirements.

Everything is passing in your local environment?

We don't have errors in our test, I forgot the requirements. We added our dev-requirements.txt. We don't have aiohttp test because we want to stay generic without aiohttp dependency.

I must say I am not understanding the last comment at all.

what does "We don't have aiohttp test because we want to stay generic without aiohttp dependency." mean? Especially when you commited aiohttp==3.8.1 to the dev-dependencies already? Do you want that or not?

And should the pytest-aiohttp from me, which already is in the async-feature branch dev-requirements.txt stay or not? Should the async tests be based on pytest-aiohttp plugin or pytest-asyncio +aiohttp ? I expected we will go pytest plugins way.

Sorry if we were not clear, I try to summarize our considerations why they led me not to choose pytest-aiohttp:

rewrite the tests faster and easier in case you want to choose to use another type library that uses async like httpx, this we meant when we said we didn't want to depend on aiohttp for testing

the tests work with Application and you have problems using the get method passing all the complete url. In fact we simply had to replace SERVICE_URL with an empty string.

We have rewritten the tests anyway using pytest-aiohttp but we remain of the idea that it is more useful to use pytest-asyncio + aiohttp.

Question: is your code in client.py/service.py compatible with httpx python package?
Or does it expect aiohttp.ClientSession() to be provided in the Client.build_async_client only?

If it is more open (best possible outcome, I don't actually know about any other than requests library being used with pyodata, see for example #154 (comment) ), then why not add both integration tests with aiohttpand httpx packages?

Another possibility is that you are ONLY talking about the aiohttp usage in the tests. But then I am still curious about the pyodata package (with this PR) compatibility matrix with async python http networking libraries.

We should check, in theory it should work with httpx as well.

rewrite tests using pytest-aiohttp

feat: update async_client tests

phanak-sap · 2022-03-14T20:39:46Z

tests/conftest.py

@@ -1,4 +1,5 @@
 """PyTest Fixtures"""
+import asyncio


You are importing direct asyncio package, while in dev dependencies is pytest-asyncio plugin.

Therefore tests are still failing.

Btw, why you prefer the asyncio/aiohttp directly as a dependency and not the pytest plugins, if it is used for testing purposes only anyway? My guess would be that pytest plugin are used by many and doing IMHO the same stuff you are doing by hand - according to readme of both plugins. In this case I think is best to use standard solution of the problem instead of own implementation. I it simple code at the moment, I know, no actual problem merging as is, by me. Just a open discussion.

I have read the comment "we remain of the idea that it is more useful to use pytest-asyncio + aiohttp" - response is there. But on separate topic just asyncio vs pytest-asyncio - it is anyway just about providing basic wrappers around event loop in the pytest fixtures, right?

We forgot to update dev-requirements.txt now you shouldn't have any problems with the test anymore. We have removed some useless code.

Yeah, with 3bab585 I have passing tests, only with one warning.

pytest_aiohttp\plugin.py:28: DeprecationWarning: The 'asyncio_mode' is 'legacy', switching to 'auto' for the sake of pytest-aiohttp backward compatibility. Please explicitly use 'asyncio_mode=strict' or 'asyncio_mode=auto' in pytest configuration file. config.issue_config_time_warning(LEGACY_MODE, stacklevel=2) -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html =========================================== 250 passed, 1 warning in 3.55s

phanak-sap · 2022-03-14T20:54:41Z

tests/test_async_client.py

+
+    assert isinstance(service_client, pyodata.v2.service.Service)
+
+    # one more test for '/' terminated url


mark at least as TODO to grab more attention in future PR if not done.

add pytest-aiohttp remove pytest-asyncio remove aiohttp

feat: update dev-requirements.txt

No duplication v1

phanak-sap · 2022-03-17T16:29:44Z

At the moment this looks good, tests are passing and anybody can play with it in the feature branch.

Merging to async-feature branch. Feature integration to master seems that will be PR and then a one big squashed commit with all the code, tests, documentation with one proper commit message.

mnunzio and others added 3 commits March 8, 2022 18:04

feat: add support for async http library aiohttp

543d175

Merge pull request #1 from mnunzio/async_python_no_duplicate_code

00ad1dc

feat: add support for async http library aiohttp

Merge pull request #2 from mnunzio/async_python_no_duplicate_code

e5b6ec1

Async python no duplicate code

phanak-sap requested review from filak-sap and phanak-sap March 9, 2022 13:42

phanak-sap added the async label Mar 9, 2022

phanak-sap changed the title ~~Async feature~~ Add async networking libraries support Mar 9, 2022

phanak-sap reviewed Mar 9, 2022

View reviewed changes

Albo90 added 2 commits March 10, 2022 23:22

Added: prefix async getattr, atom response

5f88377

added tests

a4cc84d

phanak-sap reviewed Mar 11, 2022

View reviewed changes

Albo90 force-pushed the no_duplication_v1 branch 2 times, most recently from b6de2ca to a4cc84d Compare March 11, 2022 17:41

Albo90 and others added 3 commits March 11, 2022 18:41

updated dev-requirements.txt

30470be

feat: update async_client tests

c2d662a

rewrite tests using pytest-aiohttp

Merge pull request #3 from mnunzio/no_duplication_v1

7892057

feat: update async_client tests

phanak-sap reviewed Mar 14, 2022

View reviewed changes

feat: update dev-requirements.txt

398dd24

add pytest-aiohttp remove pytest-asyncio remove aiohttp

Albo90 and others added 4 commits March 14, 2022 22:06

Merge pull request #4 from mnunzio/no_duplication_v1

bad2e41

feat: update dev-requirements.txt

feat: clean useless code

9ceb3dd

Merge branch 'Albo90:no_duplication_v1' into no_duplication_v1

cad0c92

Merge pull request #5 from mnunzio/no_duplication_v1

3bab585

No duplication v1

phanak-sap mentioned this pull request Mar 17, 2022

feature: Add async networking libraries support #208

Closed

7 tasks

phanak-sap merged commit b90e362 into SAP:async-feature Mar 17, 2022

phanak-sap mentioned this pull request Aug 19, 2022

feature: async support #225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async networking libraries support #210

Add async networking libraries support #210

Albo90 commented Mar 9, 2022 •

edited by phanak-sap

Loading

cla-assistant bot commented Mar 9, 2022 •

edited

Loading

phanak-sap commented Mar 9, 2022

Albo90 commented Mar 9, 2022

phanak-sap commented Mar 9, 2022 •

edited

Loading

phanak-sap Mar 9, 2022 •

edited

Loading

phanak-sap Mar 9, 2022

phanak-sap commented Mar 9, 2022

Albo90 commented Mar 9, 2022

phanak-sap commented Mar 10, 2022

Albo90 commented Mar 10, 2022

phanak-sap commented Mar 11, 2022 •

edited

Loading

phanak-sap commented Mar 11, 2022

phanak-sap Mar 11, 2022

phanak-sap Mar 11, 2022

Albo90 Mar 11, 2022 •

edited

Loading

phanak-sap Mar 12, 2022

mnunzio Mar 14, 2022

phanak-sap Mar 14, 2022 •

edited

Loading

mnunzio Mar 14, 2022

phanak-sap Mar 14, 2022 •

edited

Loading

mnunzio Mar 14, 2022

phanak-sap Mar 15, 2022

phanak-sap Mar 14, 2022

phanak-sap commented Mar 17, 2022 •

edited

Loading



		class Response:
		"""Representation of http response in a standard form already used by handlers"""


		assert isinstance(service_client, pyodata.v2.service.Service)

		# one more test for '/' terminated url

Add async networking libraries support #210

Add async networking libraries support #210

Conversation

Albo90 commented Mar 9, 2022 • edited by phanak-sap Loading

cla-assistant bot commented Mar 9, 2022 • edited Loading

phanak-sap commented Mar 9, 2022

Albo90 commented Mar 9, 2022

phanak-sap commented Mar 9, 2022 • edited Loading

phanak-sap Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanak-sap commented Mar 9, 2022

Albo90 commented Mar 9, 2022

phanak-sap commented Mar 10, 2022

Albo90 commented Mar 10, 2022

phanak-sap commented Mar 11, 2022 • edited Loading

phanak-sap commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Albo90 Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanak-sap Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanak-sap Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phanak-sap commented Mar 17, 2022 • edited Loading

Albo90 commented Mar 9, 2022 •

edited by phanak-sap

Loading

cla-assistant bot commented Mar 9, 2022 •

edited

Loading

phanak-sap commented Mar 9, 2022 •

edited

Loading

phanak-sap Mar 9, 2022 •

edited

Loading

phanak-sap commented Mar 11, 2022 •

edited

Loading

Albo90 Mar 11, 2022 •

edited

Loading

phanak-sap Mar 14, 2022 •

edited

Loading

phanak-sap Mar 14, 2022 •

edited

Loading

phanak-sap commented Mar 17, 2022 •

edited

Loading