python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.21k stars 343 forks source link

Update open process types #3076

Closed A5rocks closed 2 months ago

A5rocks commented 2 months ago

This should resolve the issue mentioned in Gitter

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.60%. Comparing base (0090581) to head (1688da0). Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3076 +/- ## ======================================= Coverage 99.60% 99.60% ======================================= Files 121 121 Lines 17882 17882 Branches 3214 3214 ======================================= Hits 17811 17811 Misses 50 50 Partials 21 21 ``` | [Files with missing lines](https://app.codecov.io/gh/python-trio/trio/pull/3076?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_subprocess.py](https://app.codecov.io/gh/python-trio/trio/pull/3076?src=pr&el=tree&filepath=src%2Ftrio%2F_subprocess.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3N1YnByb2Nlc3MucHk=) | `100.00% <100.00%> (ø)` | | | [src/trio/\_tools/gen\_exports.py](https://app.codecov.io/gh/python-trio/trio/pull/3076?src=pr&el=tree&filepath=src%2Ftrio%2F_tools%2Fgen_exports.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rvb2xzL2dlbl9leHBvcnRzLnB5) | `95.93% <100.00%> (ø)` | |
agronholm commented 2 months ago

The updated documentation makes no mention of PathLike – shouldn't that be fixed?

A5rocks commented 2 months ago

I figured it's fine to have more accurate documentation (type hint) and less accurate but clearer (what's written in the docstring). Both are visible in the built docs.

I can make them the same though.

A5rocks commented 2 months ago

Huh https://github.com/python-trio/trio/actions/runs/10649671109/job/29520147803#step:4:208 shouldn't be happening. I'll assume it's transient but it seems like some sort of pip bug probably? Nevermind, it's not transient. Weird.


OK it's because @actions/checkout is pulling in the merged version of this PR (I think?), which contains the test-requirements.txt changes I just merged. I'm not clear on why that's leading PyPy to fail. And I cannot reproduce this locally with the same version of PyPy on Windows...

Something I noticed while investigating caching is I'm not sure it's helping much. In fact I think we'd probably be better off without it -- the cache key seems to change every single commit which makes no sense!!

TeamSpen210 commented 2 months ago

The cache has been working a few days ago, this run for example. We'd definitely want it if possible. Could the problem be that the Cython build isn't using cache-dependency-path?

A5rocks commented 2 months ago

The cache has been working a few days ago, this run for example. We'd definitely want it if possible. Could the problem be that the Cython build isn't using cache-dependency-path?

It wasn't Cython, it was PyPy on Windows (specifically). It should be using the cache-dependency-path right because it has the same hash (c5182900c6da08ceac54233234be0232412d20dcd40abe0f886b892a46fd3c5e) within the commit for the run I linked. Maybe it's just using the hash from the current branch rather than what @actions/checkout pulls (which is the state were this PR merged)?


Here's my current thoughts: the action should only cache pip's cache directory: so there must be something bad there. However, as this run shows, it does actually look at the current test-requirements.txt file, presumably to get the hash.

Also, I'm struggling to understand why the "Setup python" step has a different hash in the cache key than the "Post Setup python" step in this run: https://github.com/python-trio/trio/actions/runs/10649416223/job/29519569640. (which I reran for debug logging and which... failed???)

TeamSpen210 commented 2 months ago

It's very mystifying. Looking at the code, the hash should be stored here, and then reloaded here, so it shouldn't be affected if the requirements file was say edited. But I was thinking maybe there could be an issue if we had another action using setup-python internally. Not sure what though.