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

Fixed unhelpful error message in from_thread_run functions. #1513

Merged
merged 16 commits into from
May 21, 2020

Conversation

guilledk
Copy link

@guilledk guilledk commented May 12, 2020

Added helpful error message in the case the user passed incorrect type of function to both "from_thread_run" functions.

Closes #1244

…e of function to both "from_thread_run" functions.
@guilledk
Copy link
Author

guilledk commented May 12, 2020

I'm not really familiar with the whole CI thing, so just point me to some docs and I'll fix the test fails.

Didn't expect a simple error message would break so many tests :P (in hindsight makes sense if they are output based)

Copy link
Member

@oremanj oremanj left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! I'm realizing in hindsight that I might've been a little overzealous in putting "good first issue" on this one; the issues are subtler than I thought at first. If you're up for another round or two of review, though, I'd be delighted to see you continue this!

The tests are failing because your change broke some supported ways of calling trio.from_thread.run_sync. If you click "Details" next to any of the failed runs, you should be able to see the error. On the details page, click on "Run tests" (the stage with the red X next to it, indicating failure) to see the output from that stage, then scroll down to see the output from pytest. One failure I see involves from_thread_run_sync(cancel_scope.cancel, trio_token=token) raising an error. That's because cancel_scope.cancel is an instance method (a method to be called on a particular object), not a function. Both are callable, but inspect cares about little differences like that.

You'll probably have a happier development experience if you run the tests on your own computer before uploading a PR, since it's much faster to see the results. Then, at least in theory, you'll only get CI failures for issues that only show up on particular operating systems or Python versions. This is described in more detail in our contributing guide: https://trio.readthedocs.io/en/stable/contributing.html#tests

Other things to add:

trio/_threads.py Outdated
@@ -377,6 +379,9 @@ def from_thread_run(afn, *args, trio_token=None):
"foreign" thread, spawned using some other framework, and still want
to enter Trio.
"""
if not inspect.iscoroutinefunction(afn):
raise AttributeError("afn must be an asynchronous function")
Copy link
Member

Choose a reason for hiding this comment

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

The user passed an argument of the wrong type, so you should raise TypeError.

In errors like this, it's also nice to mention in the message what specific thing the user sent you. For example, you could use f"{afn!r} must be an asynchronous function" to insert repr(afn) into the exception message.

Copy link
Author

Choose a reason for hiding this comment

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

Great! I knew the error message needed some work.

trio/_threads.py Outdated
@@ -377,6 +379,9 @@ def from_thread_run(afn, *args, trio_token=None):
"foreign" thread, spawned using some other framework, and still want
to enter Trio.
"""
if not inspect.iscoroutinefunction(afn):
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, identifying async functions is rather subtler than just inspect.iscoroutinefunction, because async functions can be wrapped in a way that doesn't satisfy that test. (For example, Trio functions decorated with @enable_ki_protection are implemented using a wrapper that looks synchronous. functools.partial objects don't even satisfy inspect.isfunction, let alone inspect.iscoroutinefunction. All of these are perfectly valid to pass as an "async function" as long as the function being wrapped is async.)

The best way to see if something is an async function is to call it without await and use inspect.iscoroutine on the result. Of course, if it wasn't an async function, you just executed it, so it's best to do this only in the context where you were planning to call it anyway. In this case, that would be inside unprotected_afn below.

If you're not willing to call the function, you have to get a lot more special-cased to see if it's async or not. You can look inside the .func attribute of a functools.partial object, follow the .__wrapped__ attributes generally added by function wrappers, etc, but there's no approach that will definitely succeed for any async callable.

trio/_threads.py Outdated
@@ -422,6 +427,9 @@ def from_thread_run_sync(fn, *args, trio_token=None):
"foreign" thread, spawned using some other framework, and still want
to enter Trio.
"""
if not (inspect.isfunction(fn) and not inspect.iscoroutinefunction(fn)):
Copy link
Member

Choose a reason for hiding this comment

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

As discussed above, these won't tell you what you want. Better to wait and raise an error if the result of calling the function satisfies inspect.iscoroutine. (If it does, you'll also want to close() it before returning, so you don't get an extra "coroutine was never awaited" warning.)

Copy link
Author

Choose a reason for hiding this comment

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

Just saw your comment on #1504, should I look into Runner.spawn_impl() for the "async fn or error" logic?

Copy link
Member

Choose a reason for hiding this comment

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

That would be a useful extension if you feel like it! The relevant code is toward the top of spawn_impl(), with the comments about "Give good error for [...]"

@guilledk
Copy link
Author

Thanks a lot for the feedback!, I'll work on it and send another PR!

@njsmith
Copy link
Member

njsmith commented May 13, 2020

@guilledk Note that you don't have to actually create a new PR from scratch – you can just more commits to your issue1244 branch on github, and they'll automatically appear here in this PR.

@guilledk
Copy link
Author

guilledk commented May 13, 2020

@njsmith Ok good to know.

Thanks a lot for the patience, you guys rock!

…d it to trio._util. Used it in trio.from_thread_run.
@guilledk
Copy link
Author

So following what @oremanj said in #1504, I moved the "either get a coroutine from this async function+args or raise an error explaining why it's not in the form I wanted" logic to a function in trio._util called coroutine_or_error (WIP name).

Added the check to trio._threads.from_thread_run, but its still missing the check on trio._threads.from_thread_run_sync, I'll do that one tomorrow.

Also there is some code formatting issues I have to fix.

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #1513 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #1513    +/-   ##
========================================
  Coverage   99.67%   99.67%            
========================================
  Files         107      108     +1     
  Lines       13254    13356   +102     
  Branches     1006     1012     +6     
========================================
+ Hits        13211    13313   +102     
  Misses         28       28            
  Partials       15       15            
Impacted Files Coverage Δ
trio/_core/_run.py 99.73% <100.00%> (-0.01%) ⬇️
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)
trio/_core/tests/tutil.py 100.00% <100.00%> (ø)
trio/_threads.py 100.00% <100.00%> (ø)
trio/_util.py 100.00% <100.00%> (ø)
trio/tests/test_threads.py 100.00% <100.00%> (ø)
trio/tests/test_util.py 100.00% <100.00%> (ø)
trio/testing/_trio_test.py 87.50% <0.00%> (-0.74%) ⬇️
trio/_highlevel_open_unix_stream.py 91.30% <0.00%> (-0.37%) ⬇️
trio/_highlevel_open_tcp_stream.py 96.96% <0.00%> (-0.05%) ⬇️
... and 29 more

@guilledk
Copy link
Author

Cool, all that's missing are the tests!

@guilledk guilledk changed the title Fixed unhelpful error message in from_thread_run functions. Fixed unhelpful error message in from_thread_run functions. Closes #1244 May 13, 2020
@guilledk guilledk changed the title Fixed unhelpful error message in from_thread_run functions. Closes #1244 Fixed unhelpful error message in from_thread_run functions. May 13, 2020
@guilledk
Copy link
Author

@njsmith Hey so I could use some pointers to fix the code coverage issues, I check the codecov diff and I don't seem to get were the issue is. Thanks in advance!

@pquentin
Copy link
Member

The coverage drop is in trio/socket.py: https://codecov.io/gh/python-trio/trio/pull/1513/src/trio/socket.py

Screenshot 2020-05-14 at 15 26 38

The issue is that the macOS 3.6 build was cancelled after 1m30s, I don't know why: https://github.com/python-trio/trio/pull/1513/checks?check_run_id=672946337

Closing/reopening to re-run all jobs

@pquentin pquentin closed this May 14, 2020
@pquentin pquentin reopened this May 14, 2020
@guilledk
Copy link
Author

Cool so this closes #1244

@pquentin
Copy link
Member

Great that the tests and coverage pass!

@guilledk Note that you need to edit your first message to say that, otherwise GitHub won't close the issue when we merge this.

@guilledk
Copy link
Author

@pquentin Done!

@guilledk guilledk requested a review from oremanj May 14, 2020 17:40
trio/_core/_run.py Show resolved Hide resolved
trio/_threads.py Outdated Show resolved Hide resolved
trio/_threads.py Outdated
:exc:`trio.Cancelled`, and this will propagate out into
RuntimeError: if you try calling this from inside the Trio thread,
which would otherwise cause a deadlock.
AttributeError: if no ``trio_token`` was provided, and we can't infer
one from context.
one from context. Also if ``fn`` is not a sync function.
Copy link
Member

Choose a reason for hiding this comment

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

This is leftover from your previous use of AttributeError for this condition, but you're now using TypeError.

Copy link
Member

Choose a reason for hiding this comment

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

You marked this as resolved but you didn't update the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

You still have not updated the docstring despite having marked this as resolved again. The request is to remove the text "Also if fn is not a sync function" from the description for AttributeError, because you no longer raise AttributeError in that case.

trio/_threads.py Outdated Show resolved Hide resolved
trio/_util.py Outdated Show resolved Hide resolved
trio/_util.py Outdated Show resolved Hide resolved
trio/_threads.py Outdated Show resolved Hide resolved
trio/_core/tests/test_run.py Show resolved Hide resolved
trio/tests/test_threads.py Outdated Show resolved Hide resolved
trio/tests/test_threads.py Outdated Show resolved Hide resolved
@guilledk guilledk requested review from oremanj and njsmith May 16, 2020 12:16
trio/_threads.py Outdated
:exc:`trio.Cancelled`, and this will propagate out into
RuntimeError: if you try calling this from inside the Trio thread,
which would otherwise cause a deadlock.
AttributeError: if no ``trio_token`` was provided, and we can't infer
one from context.
one from context. Also if ``fn`` is not a sync function.
Copy link
Member

Choose a reason for hiding this comment

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

You still have not updated the docstring despite having marked this as resolved again. The request is to remove the text "Also if fn is not a sync function" from the description for AttributeError, because you no longer raise AttributeError in that case.

trio/_threads.py Outdated Show resolved Hide resolved
async def f(): # pragma: no cover
pass
with ignore_coroutine_never_awaited_warnings():
with pytest.raises(TypeError, match="expecting an async function"):

Copy link
Member

Choose a reason for hiding this comment

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

You marked this as resolved but didn't change anything in your PR. Do you feel like the extra blank lines here are important for readability in a way that's related to the changes you're making here? It seems like you've added a number of them in this diff after block introducers (for, with, if, etc). That's not consistent with the style in the rest of the codebase.

@guilledk
Copy link
Author

Ok now I think I got all the formatting issues, sorry guys very used to my code style. Thanks again for the patience.

@guilledk guilledk requested a review from oremanj May 18, 2020 12:27
@oremanj
Copy link
Member

oremanj commented May 20, 2020

Thank you for your patience! I removed a few more of the extraneous blank lines, and will merge this once the CI finishes.

Copy link
Member

@oremanj oremanj left a comment

Choose a reason for hiding this comment

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

Actually, my bad -- I was just giving this another look and I don't think you added a newsfragment describing your change. I mentioned this in my initial comment but failed to follow up. See https://trio.readthedocs.io/en/latest/contributing.html#release-notes for more details. You want to create a file called newsfragments/1244.bugfix.rst that describes what changed and (briefly) why, from a user-visible perspective. Each bullet point in our release notes comes from one of these, so you can get a sense of the style by reading the release notes: https://trio.readthedocs.io/en/latest/history.html

@oremanj
Copy link
Member

oremanj commented May 20, 2020

Also, since I made some trivial commits above, you'll want to pull from your PR branch before you make additional changes. Sorry for the trouble!

@guilledk
Copy link
Author

No trouble at all! Now I understand the workflow a lot more, next time I'll take less reviews I swear haha

@guilledk guilledk requested a review from oremanj May 20, 2020 02:33
@guilledk guilledk closed this May 20, 2020
@guilledk guilledk reopened this May 20, 2020
@guilledk guilledk requested a review from oremanj May 20, 2020 11:18
@oremanj oremanj merged commit a266dfc into python-trio:master May 21, 2020
@trio-bot
Copy link

trio-bot bot commented May 21, 2020

Hey @guilledk, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂

If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:

  • Github will automatically subscribe you to notifications on all our repositories. (But you can unsubscribe again if you don't want the spam.)

  • You'll be able to help us manage issues (add labels, close them, etc.)

  • You'll be able to review and merge other people's pull requests

  • You'll get a [member] badge next to your name when participating in the Trio repos, and you'll have the option of adding your name to our member's page and putting our icon on your Github profile (details)

If you want to read more, here's the relevant section in our contributing guide.

Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you.

If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out!

@pquentin
Copy link
Member

Congratulations @guilledk! How was the experience? Would you like to work on another issue next? #1532, for example?

@guilledk
Copy link
Author

Thanks @pquentin!, Awsome experience, to be honest never wrote a test before this PR, now all my projects have tests, so I learned a thing already.

I was actually looking at #1532 to do next, I really like the trio community and technology, I'd like to keep learning about the backend and helping you guys. Right now I have project that runs on top of trio and might go into production soon so I'd like to give back!

@guilledk guilledk deleted the issue1244 branch May 21, 2020 11:31
@pquentin
Copy link
Member

@guilledk Thanks, that's great to hear! If you decide to work on #1532, please ask if you have any questions, that might avoid a round of feedback or two. :)

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.

In trio.to_thread.run, give a better error message if passed a sync function
4 participants