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

Make the mypy-based tests faster! #2664

Merged
merged 2 commits into from
Jun 25, 2023
Merged

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Jun 20, 2023

This makes some of the tests slower cause it stops using an (implicit) cache in CWD. This makes them generally faster, however.

Fixes #2662

Edit: huh, did not realize I pressed the "assignees" button!

@A5rocks A5rocks assigned jakkdl and unassigned jakkdl Jun 20, 2023
@A5rocks A5rocks requested a review from jakkdl June 20, 2023 20:32
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #2664 (1f75b1f) into master (493c915) will decrease coverage by 0.07%.
The diff coverage is 93.68%.

❗ Current head 1f75b1f differs from pull request most recent head 7be61ff. Consider uploading reports for the commit 7be61ff to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2664      +/-   ##
==========================================
- Coverage   98.15%   98.09%   -0.07%     
==========================================
  Files         118      118              
  Lines       16480    16536      +56     
  Branches     2983     3000      +17     
==========================================
+ Hits        16176    16221      +45     
- Misses        249      256       +7     
- Partials       55       59       +4     
Impacted Files Coverage Δ
trio/_tests/test_exports.py 95.00% <93.68%> (-5.00%) ⬇️

@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 20, 2023

Timeouts are annoying, I think fixing those (probably just making mypy run once without pytest timeout and share that cache whereever) should be another PR however. NVM I need to fix that to merge this...

Edit: also I see this ended up being slower by 2 minutes... huh??


# I have not researched why these are missing, should maybe create an issue
# upstream with jedi
if tool == "jedi" and sys.version_info >= (3, 11):
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 case it isn't obvious, this changed from sys.version_info >= (3, 12) to sys.version_info >= (3, 11) cause of a backport, probably.

@A5rocks A5rocks changed the title Make the mypy-based tests faster! (mostly) Make the mypy-based tests faster! Jun 21, 2023
Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

We could also set up a shared cache dir across the jobs - I think we can assume that mypy will generate the same cache on different OS's, and could then also use that in check.sh.

Not sure if https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows or https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts would be the way to go.

trio/_tests/test_exports.py Outdated Show resolved Hide resolved
ci.sh Outdated Show resolved Hide resolved
trio/_tests/test_exports.py Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member

jakkdl commented Jun 21, 2023

Timeouts are annoying, I think fixing those (probably just making mypy run once without pytest timeout and share that cache whereever) should be another PR however. NVM I need to fix that to merge this...

Edit: also I see this ended up being slower by 2 minutes... huh??

Is it still slower? Maybe mypy's cache parsing is sufficiently optimized that (only) sidestepping it doesn't actually improve it.

@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 21, 2023

Timeouts are annoying, I think fixing those (probably just making mypy run once without pytest timeout and share that cache whereever) should be another PR however. NVM I need to fix that to merge this...
Edit: also I see this ended up being slower by 2 minutes... huh??

Is it still slower? Maybe mypy's cache parsing is sufficiently optimized that (only) sidestepping it doesn't actually improve it.

Nope, that comment's out of date. It's like 1/3rd faster now. (master takes ~2 minutes, this takes ~1:20)

We could also set up a shared cache dir across the jobs - I think we can assume that mypy will generate the same cache on different OS's, and could then also use that in check.sh.

Not sure if https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows or https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts would be the way to go.

Eh, not too keen on this. Mypy's cache can lead to issues and I don't think an extra 10 seconds or so in the tests is worth the wackiness it leads to.

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

2 small changes, feel free to merge after addressing them!

trio/_tests/test_exports.py Outdated Show resolved Hide resolved
trio/_tests/test_exports.py Outdated Show resolved Hide resolved
@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 24, 2023

Rewrote history a bit to make the merge look neater, feel free to merge via merge commit (do the honors!)!

@jakkdl jakkdl merged commit 0e8a989 into python-trio:master Jun 25, 2023
30 of 33 checks passed
@A5rocks A5rocks deleted the faster-mypy-tests branch July 12, 2023 22:46
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.

mypy tests in test_exports.py are *extremely* slow.
2 participants