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

Remove call to nest_asyncio.apply in server startup #1048

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

charlesbluca
Copy link
Collaborator

Attempting to unblock some issues starting up the server with run_server(blocking=True) reported by @randerzander; seems like these crop up when we attempt to patch the event loop, interested in how CI will respond to us removing this patch.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023

Codecov Report

Merging #1048 (7d5ac33) into main (26fbe16) will increase coverage by 0.11%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
+ Coverage   81.31%   81.42%   +0.11%     
==========================================
  Files          78       78              
  Lines        4373     4372       -1     
  Branches      786      786              
==========================================
+ Hits         3556     3560       +4     
+ Misses        644      635       -9     
- Partials      173      177       +4     
Impacted Files Coverage Δ
dask_sql/server/app.py 100.00% <100.00%> (ø)
dask_sql/_version.py 35.31% <0.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Does this unblock a few tests?

@charlesbluca
Copy link
Collaborator Author

Don't think this has an impact on the server tests since those are accessing the server through the fastapi.testclient.TestClient and so we don't actually start up a uvicorn server; not sure if there's any larger reasoning behind the decision to use the TestClient there but could be worthwhile to explore actually spinning up and shutting down a server for the tests to make sure things work as expected there.

Should also note that some of the failures are pretty trivial, and just come down to differences in the expected column / dtype names, if we begin pushing on updating server support those shouldn't be too difficult to get passing again.

@ayushdg ayushdg merged commit 70c9057 into dask-contrib:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants