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 #99: "RuntimeError: This event loop is already running" in colab and notebook #100

Closed

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Jun 29, 2021

Fixes #99

Changes

  • If event loop is already running and RestClient is instantiated, try to import nest_asyncio and patch with nest_asyncio.apply(). Raise ModuleNotFoundError if nest_asyncio not installed.

Testing

  1. Test added to verify ModuleNotFoundError is raised.

Checklist

  • PR has an informative and human-readable title
  • PR is well outlined and documented. See #12 for an example
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (see CONTRIBUTING.md)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output) using numpy docstring formatting
  • Placeholder code is flagged / future todos are captured in comments
  • Reviewers requested with the Reviewers tool ➡️

@aaraney aaraney added the bug Something isn't working label Jun 29, 2021
@aaraney aaraney requested a review from jarq6c June 29, 2021 20:23
@aaraney aaraney self-assigned this Jun 29, 2021
warnings.simplefilter("ignore", category=RuntimeWarning)
with pytest.raises(ModuleNotFoundError):
# implicitly verify that `nest_asyncio` is not installed
# this test will need to change if `nest_asyncio` becomes a requirement
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to elaborate in this comment a bit more concerning the implications of nest_asyncio becoming a requirement? I assume this test will fail is it's already installed. Would that require us to remove this test or write a new test? If nest_asyncio became a requirement, would that remove the need for the test? If so, why not just make it a requirement now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you are correct -- if nest_asyncio is already installed that test will fail. My thinking was to treat this dependency differently since it isn't typically required. I agree with you about the fragility of this test, I need to rethink how to test this bug fix.

@jarq6c
Copy link
Collaborator

jarq6c commented Aug 18, 2021

@aaraney Any updates on this front?

@aaraney
Copy link
Member Author

aaraney commented Aug 18, 2021

@aaraney Any updates on this front?

I should have some time this afternoon to give this more thought. Given that there is now a merge conflict, I will likely close this and ref it in a new PR that is based off the current HEAD.

@aaraney
Copy link
Member Author

aaraney commented Aug 24, 2021

With the re-opening of this PR as #130, I am going to close this issue.

@aaraney aaraney closed this Aug 24, 2021
jarq6c added a commit that referenced this pull request Aug 25, 2021
Fix #99: "RuntimeError: This event loop is already running" in colab and notebook. Re-PR of #100
aaraney added a commit to aaraney/hydrotools that referenced this pull request Oct 6, 2021
@jarq6c jarq6c linked an issue Oct 8, 2021 that may be closed by this pull request
@aaraney aaraney deleted the fix-restclient-loop-already-created branch May 19, 2023 01:24
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
2 participants