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

Fix event loop errors #650

Merged
merged 9 commits into from
Jun 20, 2023
Merged

Conversation

janjagusch
Copy link
Collaborator

No description provided.

@janjagusch janjagusch added the bug Something isn't working label Jun 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Patch coverage: 76.47% and project coverage change: +0.11 🎉

Comparison is base (282f8ab) 83.14% compared to head (27e8fc4) 83.26%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
+ Coverage   83.14%   83.26%   +0.11%     
==========================================
  Files          78       78              
  Lines        6153     6184      +31     
==========================================
+ Hits         5116     5149      +33     
+ Misses       1037     1035       -2     
Impacted Files Coverage Δ
quetz/_version.py 0.00% <0.00%> (ø)
quetz/main.py 86.46% <0.00%> (ø)
quetz/pkgstores.py 68.01% <81.25%> (+2.57%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@janjagusch janjagusch marked this pull request as ready for review June 16, 2023 11:47
quetz/pkgstores.py Outdated Show resolved Hide resolved
Comment on lines 305 to 311
self.fs = s3fs.S3FileSystem(
key=key, secret=secret, asynchronous=True, client_kwargs=client_kwargs
key=key,
secret=secret,
asynchronous=True,
client_kwargs=client_kwargs,
)
self.fs._loop = get_loop()
Copy link
Collaborator Author

@janjagusch janjagusch Jun 16, 2023

Choose a reason for hiding this comment

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

@martindurant, I hope you don't mind that I'm looping you in here. This is a follow-up from our conversation in fsspec/filesystem_spec#1290. You suggested to explicitly pass the loop argument to the file system constructor. However, this doesn't seem possible, since whenever asynchronous=True then self._loop=None (see here).

The only solution I could come up with is to set the self._loop attribute after the object has been instantiated. Is this a bug in fsspec or am I doing something wrong?

Thanks a lot for your help!

Choose a reason for hiding this comment

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

Perhaps the easiest is to follow the pattern in a test: https://github.com/fsspec/s3fs/blob/main/s3fs/tests/test_s3fs.py#L2000

I am sure this is also documented, but where to look between s3fs's and fsspec's docs isn't always clear. Help always appreciated...

Choose a reason for hiding this comment

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

but again, the best thing you can do is actually create the filesystem object within a coroutine, in which case the asynchronous and loop kwargs should not be necessary (but you cannot call any sync-API methods)

@janjagusch janjagusch merged commit 13f588c into mamba-org:main Jun 20, 2023
ivergara pushed a commit to ivergara/quetz that referenced this pull request Jun 21, 2023
* Add pytest-asyncio as dependency.

* Explicitly pass loop.

* Pass loop outside of constructor.

* Debug.

* Revert "Debug."

This reverts commit 3fba70b.

* Implement s3 pkgstore async correctly.

* Implement azure pkgstore async correctly.

* Implement gcs pkgstore async correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants