opensearch-project / opensearch-py

Python Client for OpenSearch
https://opensearch.org/docs/latest/clients/python/
Apache License 2.0
337 stars 168 forks source link

[BUG] Async model usage needs to import private `_async` module #635

Open pattrickrice opened 9 months ago

pattrickrice commented 9 months ago

What is the bug?

A clear and concise description of the bug.

If I want to use AsyncDocument for example, it seems like I need to import from a private module (private indicated by _ prefix.

from opensearchpy._async.helpers.document import AsyncDocument

How can one reproduce the bug?

Steps to reproduce the behavior.

from opensearchpy._async.helpers.document import AsyncDocument

What is the expected behavior?

If this is not expected to be private, the suggestion would be to remove the _. Otherwise it appears that use of the async module is discouraged.

from opensearchpy.async.helpers.document import AsyncDocument

What is your host/environment?

Operating system, version. n/a

Do you have any screenshots?

If applicable, add screenshots to help explain your problem. n/a

Do you have any additional context?

Add any other context about the problem.

n/a

dblock commented 9 months ago

Yes, this looks incorrect. Would appreciate a PR. Are there other types in this case? Does anything have to be _async?

drewbrew commented 8 months ago

from opensearchpy.async.helpers.document import AsyncDocument

This is invalid because async is a reserved keyword. Either make it async_ or something else entirely, I think.

pattrickrice commented 8 months ago

Oh interesting, I hadn't considered that. Is there a reason the async classes aren't exported in the main barrel export? https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/__init__.py

It seems like most of them are prefixed with Async{ClassName}; so, I'd assume there wouldn't be conflicts in adding them there?

dblock commented 8 months ago

Thanks for pointing this out @drewbrew. I guess I don't know the answer to this, let's work backwards from what we want?

pattrickrice commented 7 months ago

Just verified that yes naming a module async will not work:

Traceback (most recent call last):
  File "<frozen runpy>", line 189, in _run_module_as_main
  File "<frozen runpy>", line 112, in _get_module_details
  File "/private/tmp/test/__init__.py", line 1
    import async
           ^^^^^
SyntaxError: invalid syntax

Hmm working backwards... It would be interesting if async stuff were defined next to their sync counter parts rather than being grouped in their own directory? That's sort of a larger reorganization proposal. Other easier alternatives could be _ as a suffix (async_) which wouldn't conventionally mean private in python, expanding async to asynchronous, or using a multi-word like async_opensearch.

dblock commented 7 months ago

@pattrickrice Is this based on the code duplication? Much of it is generated, or should be, so I'd start by deleting all the API implementations and focusing on a new code generator from spec that spits out any layout that you think makes sense.