AsyncGenerator → AsyncIterator by DimitriPapadopoulos · Pull Request #2381 · zarr-developers/zarr-python

DimitriPapadopoulos · 2024-10-15T17:14:25Z

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos · 2024-10-15T17:14:25Z

Fixes#2377.

See microsoft/pyright#4741.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

paraseba · 2024-10-16T11:52:02Z

I think a better type for these functions would be:

asyncdeflist(self) ->AsyncIterator[str]:

Most implementations probably won't need these functions to be async, but it's a more general type for some implementation that may need it.

DimitriPapadopoulos · 2024-10-16T12:35:24Z

I think a better type for these functions would be:
asyncdeflist(self) ->AsyncIterator[str]:

The roadmap defines the Store API as follows and should be updated:

asyncdeflist(self) ->List[str]:
... # required for listable storesasyncdeflist_prefix(self, prefix: str) ->List[str]:
... # required for listable storesasyncdeflist_dir(self, prefix: str) ->List[str]:
... # required for listable stores

~~All current child classes use the prefix argument. I don't think it should be removed.~~

As for the return value, not sure why it changed from List[str] to AsyncGenerator[str, None] in #1844. Switch back to AsyncGenerator[str]?

paraseba · 2024-10-16T12:40:41Z

~~All current child classes use the prefix argument. I don't think it should be removed.~~

Sorry, I just missed the argument in the example, definitely we shouldn't drop it. My point was about returning AsyncIterator instead of AsyncGenerator, the later is more of an implementation detail. We definitely don't want list because that would force the whole thing to live in memory at once.

DimitriPapadopoulos · 2024-10-16T12:47:38Z

Meanwhile, I tried to revert the return type from AsyncGenerator[str, None] to AsyncGenerator[str], to see what happens.

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

paraseba · 2024-10-16T12:51:09Z

As for the AsyncGenerator → AsyncIterator change, how about addressing it in an issue and a separate PR?

It's definitely not a big deal, AsyncGenerator[str] also works. But, doing it in separate PR would require changing the abc and every implementation twice.

DimitriPapadopoulos · 2024-10-16T12:51:59Z

OK, let me try then. It's just that's I'll have to modify all child classes.

paraseba · 2024-10-16T22:17:54Z

- if p.is_file():
- yield str(p).replace(to_strip, "")
+ allfiles = [str(p).replace(to_strip, "") for p in self.root.rglob("*") if p.is_file()]
+ return (onefile for onefile in allfiles)


we don't want to materialize the full allfiles list in memory. Can you use async for instead? Here is an example of how you could do this:
fromcollections.abcimportAsyncIteratorfromabcimportABC, abstractmethodclassFoo(ABC): @abstractmethodasyncdeffoo(self) ->AsyncIterator[str]: ... classBar(Foo): asyncdefhelper(self) ->AsyncIterator[str]: raiseValueError("") asyncdeffoo(self) ->AsyncIterator[str]: return (xasyncforxinawaitself.helper())
This code should work and mypy successfully, and doesn't materialize the results of calling helper.

I'm a little bit lost here.
What we have here are functions that yield. That's a generator. Why create an iterator?
I'm afraid I don't have enough experience with async and no time to learn it in the short term. For example, I'm not even sure how to identify iterables and awaitables.
I suggest We keep AsyncGenerator for now. What would be the benefit of an AsyncIterator in this context?

jhamman · 2024-11-05T05:16:25Z

@DimitriPapadopoulos - are you planning to return to this?

DimitriPapadopoulos · 2024-11-05T06:39:26Z

I tried briefly, but I cannot get typing to work after rebasing. I need to take a thorough look.

DimitriPapadopoulos · 2024-11-07T15:11:25Z

How to correctly specify type hints with AsyncGenerator and AsyncContextManager

jhamman

I'm happy here (except for the requested docstring changes).

jhamman · 2024-11-08T16:42:28Z

+ Notes
+ -----
+ This method should be async,
+ `How to correctly specify type hints with AsyncGenerator and AsyncContextManager <https://stackoverflow.com/questions/68905848/how-to-correctly-specify-type-hints-with-asyncgenerator-and-asynccontextmanager>`_
+ explains why it is not.


I'd like to move this to a comment rather than the public docstring. Most implementations will inherit this and end users don't need to know about this implementation detail.

Yes, I wasn't sure about it. I put it in the documentation because it documents an inconsistency in function signatures.

Ideally, I would document this using something similar to disabling Pylint warning W0236:
# pylint: disable=invalid-overridden-method
Unfortunately there's no equivalent rule in ruff, as far as I can see.

DimitriPapadopoulos force-pushed the async branch 3 times, most recently from e86ee25 to 694af4bCompare October 16, 2024 06:23

DimitriPapadopoulos marked this pull request as ready for review October 16, 2024 06:29

paraseba reviewed Oct 16, 2024
View reviewed changes

Comment threadsrc/zarr/abc/store.py Outdated

DimitriPapadopoulos force-pushed the async branch 2 times, most recently from 46dc84b to 52e66c9Compare October 16, 2024 12:13

DimitriPapadopoulos force-pushed the async branch 4 times, most recently from 844ccad to 8d3ead3Compare October 16, 2024 21:02

paraseba reviewed Oct 16, 2024
View reviewed changes

Comment threadsrc/zarr/abc/store.py Outdated

paraseba reviewed Oct 16, 2024
View reviewed changes

DimitriPapadopoulos marked this pull request as draft October 17, 2024 07:19

DimitriPapadopoulos force-pushed the async branch from 8d3ead3 to f2d2683Compare October 17, 2024 07:20

DimitriPapadopoulos mentioned this pull request Oct 18, 2024
Inconsistent listdir() and rmdir() method signature across class hierarchy #890
Closed

DimitriPapadopoulos force-pushed the async branch 8 times, most recently from 97d384a to 2efe135Compare October 19, 2024 10:19

DimitriPapadopoulos force-pushed the async branch from 2efe135 to a0b8bf0Compare October 19, 2024 12:00

DimitriPapadopoulos marked this pull request as ready for review October 19, 2024 12:05

DimitriPapadopoulos force-pushed the async branch 2 times, most recently from 1ae24a6 to 93ff6f2Compare October 20, 2024 09:54

DimitriPapadopoulos force-pushed the async branch 2 times, most recently from 5209fac to c90c23aCompare November 4, 2024 09:47

DimitriPapadopoulos force-pushed the async branch 2 times, most recently from 80c5fff to 655a149Compare November 5, 2024 06:34

DimitriPapadopoulos force-pushed the async branch from 655a149 to 56764daCompare November 5, 2024 06:53

DimitriPapadopoulos marked this pull request as draft November 5, 2024 06:53

DimitriPapadopoulos force-pushed the async branch from 56764da to 8c48380Compare November 5, 2024 06:58

DimitriPapadopoulos mentioned this pull request Nov 5, 2024
AsyncGenerator should not yield None#2464
Merged
6 tasks

DimitriPapadopoulos force-pushed the async branch 3 times, most recently from 113e9cd to 6c8dd5cCompare November 7, 2024 15:22

DimitriPapadopoulos changed the title ~~Make methods of abstract base class async~~AsyncGenerator → AsyncIteratorNov 7, 2024

DimitriPapadopoulos force-pushed the async branch 2 times, most recently from 1a824a5 to 3fa0e1dCompare November 7, 2024 17:31

DimitriPapadopoulos marked this pull request as ready for review November 7, 2024 17:31

DimitriPapadopoulos force-pushed the async branch from 3fa0e1d to 667ec8dCompare November 7, 2024 17:32

jhamman approved these changes Nov 8, 2024
View reviewed changes

DimitriPapadopoulos force-pushed the async branch from 667ec8d to 63b0438Compare November 8, 2024 17:39

AsyncGenerator → AsyncIterator
782bfa4

DimitriPapadopoulos force-pushed the async branch from 63b0438 to 782bfa4Compare November 8, 2024 17:40

jhamman merged commit 693e11c into zarr-developers:mainNov 8, 2024

DimitriPapadopoulos deleted the async branch November 8, 2024 21:43

jhamman mentioned this pull request Dec 3, 2024
Feat/latency store #2474
Merged
6 tasks