Skip to content

feature: Add async networking libraries support  #208

Closed

Description

@Albo90

TOP LEVEL FEATURE TASK LIST:

Hi,
I used this library recently, it's fantastic very convenient but it doesn't support async pattern.
I and my collegue modified the library to support aiohttp, this is the link: https://github.com/Albo90/python-pyodata/tree/async_python
If you are interested, we can collaborate to integrate it in your project.
We are going to add test and more documentation in a couple of days.

Thanks

Activity

  1. filak-sap commented on Mar 8, 2022

    @filak-sap
    Contributor

    Impressive. Unfortunately, since you copied all files, I cannot see the needed changes. Can you somehow summarize what needs to be changed?

  2. Albo90 commented on Mar 8, 2022

    @Albo90
    ContributorAuthor

    Impressive. Unfortunately, since you copied all files, I cannot see the needed changes. Can you somehow summarize what needs to be changed?

    Hi filak-sap,
    we had to create a bit of redundancy for our modifications otherwise it's really difficult for us to integragate async. I try to describe them grouping for file.

    async_client

    1. Definition of _ new_ method accord to async strategy.
    2. Definition of _fetch_metadata with get implementation accord to async.

    AsyncService

    1. Modification of execute method into OdataHttpRequest, we use "async with" to create request and build response in aim to avoid excessive ridefionition of project structure.
    2. Modification of http_get method.

    If you want, we can make a reasoning to build other ways of integration

  3. filak-sap commented on Mar 8, 2022

    @filak-sap
    Contributor

    Is there a way how to avoid huge code duplications?

  4. Albo90 commented on Mar 8, 2022

    @Albo90
    ContributorAuthor

    Is there a way how to avoid huge code duplications?

    Yes, in my option there is possibility to avoid creation of files (async_client, asyncService) rebuilding the existent methods.

  5. Albo90 commented on Mar 8, 2022

    @Albo90
    ContributorAuthor

    @filak-sap we published a new version without duplication. The version is in this branch: https://github.com/Albo90/python-pyodata/tree/no_duplication_v1. The library has a little syntax changement when it is used with async (pay attention to method getattr in example).

    Sync version:

    import requests
    from pyodata.client import Client
    
    client = Client('http://example.com', requests)
    entities = client.entity_sets.resource.get_entities().execute()
    for entity in entities:
          print(entity.attr_name)
    
    print(entities)

    Async version:

    import asyncio
    import aiohttp
    from pyodata.client import Client
    
    async def main():
        async with aiohttp.ClientSession() as client:
            service = await Client.build_async_client('http://example.com', client)
            entities = await service.entity_sets.resource.get_entities().select('key').async_execute()
            for entity in entities:
                attr_name = await entity.getattr('attr_name')
                print(attr_name)
            return entities
    
    
    async_entities = asyncio.run(main())
    print(async_entities)
  6. phanak-sap commented on Mar 9, 2022

    @phanak-sap
    Contributor

    Support of both requests lib and aiohttp lib is definitely a great idea. I had it on my personal backlog for "experiment someday", so nice to see it done and working :) Great job even just as a submitted proposal.

    I had read through the issue comments and the commits:
    Albo90@543d175
    Albo90@00ad1dc
    Albo90@e5b6ec1

    Few high-level questions:

    1. "we can make a reasoning to build other ways of integration" - do you have an alternative idea to compare with the code?
    2. There is no new test code in the three commits. Could you pls elaborate how would you like to approach unit testing of this new feature? From what I see, pytest-aiohttp seems to be the choice, but I franky only started few weeks ago to look into the asyncio library itself, so I am not an educated one. Note: I also want to create a dockerized true odata v2/v4 test service as a target for integration testing, but that will take some time and is different use case anyway.
    3. Proposal, open for discussion: Why not split the async feature in two distinctive clients modules, since even in the commits the async code is on top of the standard usage? e.g. instead service = await Client.build_async_client for example something like from pyodata.client import AsyncClient or from pyodata.async_client - as a kind of Adapter class for async networking libraries (we should keep pyodata networking library agnostic as an idea, even that we currently are sure only about the requests lib). In the future we will have pyodata.v2.client and pyodata.v4.client probably as well, with lots of shared code across all four combinations of odata V2/V4 + standard/async. My gut feeling is to go for maintainability and be specific for library user what he is actually using and the existing modules will not grow bigger and bigger. Yes, it is like in the first proposal https://github.com/Albo90/python-pyodata/tree/async_python but there the async_service.py was a really true duplicate, with no shared code reused... So I am imagining somehow merge together this module structure and the three commits small code changes. But.. again.. just an idea, perhaps wrong one.

    Feature work and Releases

    I would at the moment not go into creating PR to master, but rather create new "async" feature branch and do all the needful (code, tests, documentation) there, so pyodata is not stuck in situation where we must release a bugfix with half-baked async feature.. so far there was no need for maintenance branches and we do a flat versioning "from master".

    If we could also somehow cover Odata V4 as well, then we could even go in a Pyodata 2.0.0 release with possibility of backward-incompatible changes. Async networking and v2+v4 are big enough features for such version bump.

    Feel free to create a PR for code review to such new feature branch.

  7. Albo90 commented on Mar 9, 2022

    @Albo90
    ContributorAuthor

    Hi @phanak-sap,

    1. The expression was about first version when we created async_client and async_service duplicating a lot of code.
    2. We are working about test code, we are studying your test set to have a compliance. I think that we will add them in a couple of days.
    3. I understand your point of view and we agree about it, but in my opinion to realize it we have to pay attention to position of "execute" method, moving it maybe we have a possibility. We will think about this approach. We stay in touch !!

    For PR I haven't permission to create new branch. You should give me permission or create branch for me. Thanks.

  8. phanak-sap commented on Mar 9, 2022

    @phanak-sap
    Contributor

    Created new feature branch https://github.com/SAP/python-pyodata/tree/async-feature

    I have also marked this and odata V4 enhancement at the moment as target for 2.0.0 Milestone, since it is the safest road possible. Maybe will not be needed and will go out in some 1.X version if both features will not be backward-incompatible (which will not force us to maintain 1.x and 2.x for considerable time together).

  9. added this to the 2.0.0 milestone on Mar 9, 2022
  10. changed the title [-]Add support async Http library[/-] [+]Add async networking libraries support [/+] on Mar 9, 2022
  11. changed the title [-]Add async networking libraries support [/-] [+]feature: Add async networking libraries support [/+] on Mar 9, 2022
  12. self-assigned this
    on Aug 19, 2022
  13. modified the milestones: 2.0.0, 1.10.0 on Aug 28, 2022
  14. linked a pull request that will close this issuefeature: async support #225on Aug 28, 2022
  15. removed this from the 1.10.0 milestone on Apr 20, 2023
  16. liyakun commented on May 31, 2023

    @liyakun

    Hello @phanak-sap, may I ask whether you have any plan to include the documentation etc. to complete this new feature? Thanks.

  17. phanak-sap commented on May 31, 2023

    @phanak-sap
    Contributor

    Hi @liyakun,
    yes. But basically all I want is to move the initialization from tests to docs, and still forgetting that :) The feature itself is finished and released, so feel free to try it. Take https://github.com/SAP/python-pyodata/tree/master/tests/integration/networking_libraries as inspiration for initiliazation.

  18. liyakun commented on May 31, 2023

    @liyakun

    @phanak-sap thanks a lot, I will have a try.

  19. phanak-sap commented on Dec 19, 2023

    @phanak-sap
    Contributor

    support tasks finally done.

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

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions