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

refactor: drop futures_util dependency #3608

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 29, 2023

See #1632 (comment)

Draft based on #3599

@davidhewitt Skip changelog?

@wyfo wyfo marked this pull request as draft November 29, 2023 23:57
Copy link

codspeed-hq bot commented Nov 30, 2023

CodSpeed Performance Report

Merging #3608 will degrade performances by 25.3%

Comparing wyfo:remove_futures (830ed2f) with main (81ad2e8)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 76 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main wyfo:remove_futures Change
list_via_extract 246.1 ns 329.4 ns -25.3%
list_via_downcast 153.9 ns 126.1 ns +22.03%

@wyfo wyfo changed the title Remove futures refactor: drop futures_util dependency Nov 30, 2023
@wyfo wyfo force-pushed the remove_futures branch 3 times, most recently from 27f63b0 to c474531 Compare November 30, 2023 02:32
@messense messense added the CI-skip-changelog Skip checking changelog entry label Nov 30, 2023
@wyfo wyfo force-pushed the remove_futures branch 3 times, most recently from 1731c6d to b86bce4 Compare November 30, 2023 03:10
@wyfo wyfo mentioned this pull request Nov 30, 2023
@alex
Copy link
Contributor

alex commented Nov 30, 2023

thank you!

@wyfo wyfo force-pushed the remove_futures branch 5 times, most recently from 7621d30 to d48c8e9 Compare December 3, 2023 10:46
@wyfo wyfo marked this pull request as ready for review December 3, 2023 10:46
@wyfo
Copy link
Contributor Author

wyfo commented Dec 3, 2023

I've rebased on the last version of #3599, it should be mergeable right after #3599 merge

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Second commit looks good to me. The Clippy issues seem to stem from #3599

@wyfo
Copy link
Contributor Author

wyfo commented Dec 3, 2023

It seems to me that it's not a clippy issue but a benchmark issue (as written in the email I received). Clippy jobs are just cancelled because of benchmark failure (same in #3599).
However, I don't understand the error:

  INTERNALERROR>     from .pyo3_pytests import *
  INTERNALERROR> ModuleNotFoundError: No module named 'pyo3_pytests.pyo3_pytests'

I cannot reproduce it on my computer.

@adamreichold
Copy link
Member

It seems to me that it's not a clippy issue but a benchmark issue

There is definitely a Clippy issue here (but our logs collapse even the output of the failing invocations by default), c.f.

image

@wyfo
Copy link
Contributor Author

wyfo commented Dec 3, 2023

I think the 🤦‍♂️ emoji was invented for this moment... I've updated both PR.

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.

Thanks for cleaning up the dependency! Based on the codecov report I guess we don't have a test for the panic-inside-future case? Maybe check and if needed add one please?

After that, good to merge IMO 👍

@wyfo
Copy link
Contributor Author

wyfo commented Dec 4, 2023

Should be better now, but there was a Codecov issue in the CI:

[2023-12-04T16:14:41.932Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

@davidhewitt Do you want to relaunch coverage job?

src/coroutine.rs Outdated
@@ -1,21 +1,19 @@
//! Python coroutine implementation, used notably when wrapping `async fn`
//! with `#[pyfunction]`/`#[pymethods]`.
use std::task::Waker;
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed this only now, but I think this should be sorted into the use tree below with Content and Poll.

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 will do the modification.
However, maybe we should consider using nightly rustfmt someday. For example, in all my projects, I add the following .rustfmt.toml:

group_imports = "StdExternalCrate"
imports_granularity = "Crate"

Copy link
Member

Choose a reason for hiding this comment

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

My concern would be that this makes it harder for contributors to use a stable toolchain if it's trying to conflict with the nightly rustfmt settings, but I haven't explored this. We could certainly consider it.

@wyfo
Copy link
Contributor Author

wyfo commented Dec 4, 2023

Previous pipeline showed that some tests was not deterministic, so I replace 1s sleeps by 999s.

@davidhewitt davidhewitt added this pull request to the merge queue Dec 4, 2023
Merged via the queue into PyO3:main with commit 16ae0e2 Dec 4, 2023
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants