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

feat: add coroutine __name__/__qualname__ #3588

Merged
merged 1 commit into from
Nov 26, 2023
Merged

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 20, 2023

Relates to #1632

Copy link

codspeed-hq bot commented Nov 20, 2023

CodSpeed Performance Report

Merging #3588 will not alter performance

Comparing wyfo:coroutine_name (781b9e3) with main (1203921)

Summary

✅ 78 untouched benchmarks

@davidhewitt
Copy link
Member

@wyfo if you can rebase this please I'll get around to reviewing as soon as possible after 👍

@wyfo wyfo force-pushed the coroutine_name branch 5 times, most recently from 9502795 to 5cafd8b Compare November 23, 2023 07:47
src/impl_/coroutine.rs Outdated Show resolved Hide resolved
src/impl_/wrap.rs Outdated Show resolved Hide resolved
src/coroutine.rs Outdated
#[getter]
fn __name__(&self) -> PyResult<Py<PyString>> {
self.name
.clone()
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it is worth it, but using clone_ref would avoid the implicit check for the GIL.

src/coroutine.rs Outdated
.as_ref()
.map_or(Ok("<coroutine>"), |n| n.as_ref(gil).to_str())
.unwrap();
let message = format!("coroutine {} was never awaited", qualname);
Copy link
Member

Choose a reason for hiding this comment

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

Does this only happen if it was never awaited or also if it was not polled to completion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to me that the warning was always raised when the coroutine was not run to completion (even partially), but I've checked again, and I was wrong.
I will fix that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, instead of testing self.future.is_some(), I should test self.waker.is_none(), as it's always Some(_) when the coroutine has been polled at least once.

src/coroutine.rs Outdated
Comment on lines 169 to 170
.map_or(Ok("<coroutine>"), |n| n.as_ref(gil).to_str())
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.map_or(Ok("<coroutine>"), |n| n.as_ref(gil).to_str())
.unwrap();
.and_then(|n| n.as_ref(gil).to_str().ok())
.unwrap_or("<coroutine>");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion change the effect of the code. I wanted to panic in case of to_str error, but panicking may not be the best for a warning, you're right.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Ok, this diff is now nicely focussed just on the async code so I've given it a full review 👍

I agree with @adamreichold's points and also added a couple of brief thoughts of my own.

call = quote! {{
let future = #call;
_pyo3::impl_::coroutine::new_coroutine(
_pyo3::types::PyString::new(py, stringify!(#python_name)).into(),
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be worth interning here using our intern! macro, and changing new_coroutine to take N: IntoPy<Py<PyString>>.

Comment on lines 18 to 16
pub fn coroutine_qualname<'py>(
py: Python<'py>,
module: Option<&PyModule>,
name: &str,
) -> &'py PyString {
match module.and_then(|m| m.name().ok()) {
Some(module) => PyString::new(py, &format!("{}.{}", module, name)),
None => PyString::new(py, name),
}
}

pub fn method_coroutine_qualname<'py, T: PyClass>(py: Python<'py>, name: &str) -> &'py PyString {
let class = T::NAME;
let qualname = match T::MODULE {
Some(module) => format!("{}.{}.{}", module, class, name),
None => format!("{}.{}", class, name),
};
PyString::new(py, &qualname)
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, to avoid Python string creation with every coroutine, can this be done lazily (by e.g. storing a reference to T::type_object, or taking a function pointer, or storing a reference to the module etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Actually, methods __qualname__ could be interned too, as class name and module name are static. The issue is for free functions, because it would requires to store the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've finally used a qualname_prefix instead of passing the __qualname__ directly.
The constructor parameter may be modified when it will be exposed (maybe using Option<Cow<'static, str>> instead of Option<&'static str> for example), but for now, it's adapted to the macro and that's enough.

src/coroutine.rs Outdated
Comment on lines 164 to 165
if self.future.is_some() {
Python::with_gil(|gil| {
Copy link
Member

Choose a reason for hiding this comment

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

This structure reminds me of #3386 (comment).

I'm not entirely sure if it's a blocker to not have try_with_gil or some other guard to avoid potential nasty crashes here. We should have a little think about that before merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand how could with_gil crash here, but I know that Drop::dorp is not the best place to put this warning. In fact, it should go in __del__ (the same as CPython coroutines), but it's not supported yet in PyO3.
Maybe we can postpone the warning implementation until #2479 is resolved, as this feature is quite nonessential? We can also already write the __del__ method, as it has no effect for now, and it will work as a side effect of the __del__ support.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh that's a good suggestion, I should really pick up __del__ support again soon. Are you suggesting split this PR in two; merge the names now and keep the warning as a draft pr for the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened an issue for the warning.

@wyfo wyfo changed the title feat: add coroutine __name__/__qualname__ and not-awaited warning feat: add coroutine __name__/__qualname__ Nov 25, 2023
@wyfo wyfo changed the title feat: add coroutine __name__/__qualname__ feat: add coroutine __name__/__qualname__ Nov 25, 2023
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks! Will follow up with discussion on the issue.

@davidhewitt davidhewitt added this pull request to the merge queue Nov 26, 2023
Merged via the queue into PyO3:main with commit d8002c4 Nov 26, 2023
37 checks passed
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