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

Add pytest-split and pytest-xdist #338

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

pvandyken
Copy link
Contributor

@pvandyken pvandyken commented Sep 16, 2023

pytests-xdist allows multi-core parallelism on tests. pytest-split splits our tests into equally timed folds so that we can run everything across multiple workers. Hopefully the end result is a drastic increase decrease (5-10 fold) in our test timings

@pvandyken pvandyken added the maintenance Updates or improvements that do not change functionality of the code label Sep 16, 2023
Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

This one largely looks good to me, with one comment on the actions (can leave for a separate PR). With the test splitting, it looks like it is setting up the environment for each python version + split combination. Would need to be tested, but could consider adding a new step after quality and before testing that sets up and caches the environment for each python version.

EDIT: On a second look, could also set up all of the python versions once before quality is run - looks like py3.11 was the only one that didn't need to be set up again. Also makes sense since this was the version used in the quality step.

Copy link
Contributor

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

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

LGTM

@pvandyken
Copy link
Contributor Author

@kaitj I setup up your precaching and took the opportunity to simplify some of the workflow. I'm now using the venv caching provided natively by setup-python which seems to work well (the only "weird" thing is that you need to install poetry before setup python, but that doesn't actually cause any real problems). I also cache the poetry install now, which saves a bit of time. Since this is more than a simple edit, I'm refreshing the review request

@pvandyken pvandyken requested review from tkkuehn and kaitj September 21, 2023 15:55
Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

This lgtm! That is weird about the poetry / python order.

@pvandyken
Copy link
Contributor Author

This lgtm! That is weird about the poetry / python order.

The reason is because setup-python uses the poetry executable to figure out where dependencies are being installed so that it can set the cache path. So it's a perfectly rational explanation, just weird if you're looking at the workflow without knowing that

Copy link
Contributor

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

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

Looks good to me, with the (not new) note that it would eventually be nice to package the "Clone repo" through "Install dependencies" that get reused with minor differences into some reusable package

@pvandyken
Copy link
Contributor Author

Looks good to me, with the (not new) note that it would eventually be nice to package the "Clone repo" through "Install dependencies" that get reused with minor differences into some reusable package

@kaitj, what repository are you building your reusable github actions in?

@kaitj
Copy link
Contributor

kaitj commented Sep 21, 2023

I have been building them here just so that they are centralized, following an entity-value naming scheme, but open to changes. Also with release tags, so that downstream workflows can call a specific version.

@pvandyken pvandyken merged commit db24a2d into khanlab:main Sep 22, 2023
27 checks passed
@pvandyken pvandyken deleted the maint/test-boosting branch September 22, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants