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.

Sign up for GitHub

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

Jump to bottom

Reduce memory use during tests and relax memory limit #2418

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Reduce memory use during tests and relax memory limit #2418

merged 5 commits into from
Nov 26, 2024

Conversation

samtygier-stfc
Copy link
Collaborator

@samtygier-stfc samtygier-stfc commented Nov 26, 2024 •
edited
Loading

Issue

Closes #2417

Description

Prevent a large number of python processes being started when running pytest

Previous Pytest would start a worker for each physical cpu core (though pytest-xdist), each of these could start a multiprocessing pool with a process for each logical core (hyperthread). On a 8-core 16-thread, this could launch many python processes (over 60) and use 8GB of RAM.

This PR does several things

  • Limit size of multiprocessing pool to 2 during tests (it is controlled by a setting for the application)
  • Limit number of pytest workers to 4
  • Keep all tests that use multiprocess in a single pytest worker (using xdist loadgroup)

Testing & Acceptance Criteria

Run
make test

Documentation

Release notes

@samtygier-stfc
Limit number of multiprocessing processes used for running tests
60f0fe9 
On a 16 logical core linux system this reduces the peak memory used during testing from ~8.5GB to 4GB
@samtygier-stfc
Limit number of pytest workers used for running tests
1f7cd4b 
On a 16 logical core linux system this reduces the peak memory used during testing from 4GB to 3GB
@samtygier-stfc
Use pytest-xdist groups to prevent multithreading start in multiple p…
88094c3 
…ytest workers
@samtygier-stfc
Reduce reserved memory to 5%
296a34d 
Our systems have more RAM than they did when this was set at 12.5%
@samtygier-stfc
Release notes
1a78913
@coveralls
Copy link

Coverage Status

coverage: 74.172% (-0.01%) from 74.185%
when pulling 1a78913 on 2417-pytest-memory
into c7980a2 on main.

@samtygier-stfc samtygier-stfc marked this pull request as ready for review November 26, 2024 16:02
@ashleymeigh2 ashleymeigh2 self-requested a review November 26, 2024 16:44
ashleymeigh2
ashleymeigh2 reviewed Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@ashleymeigh2 ashleymeigh2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good! Limiting multiprocessing and pytest workers is a sensible solution , and everything seems in order. 👍

ashleymeigh2
ashleymeigh2 approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@ashleymeigh2 ashleymeigh2 left a comment

Choose a reason for hiding this comment

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

Changes look good! Limiting multiprocessing and pytest workers is a sensible solution , and everything seems in order. 👍

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Nov 26, 2024
@samtygier-stfc samtygier-stfc merged commit 1a1937b into main Nov 26, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the 2417-pytest-memory branch November 26, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashleymeigh2 ashleymeigh2 ashleymeigh2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pytest fails due to "not enough physical memory available"
3 participants
@samtygier-stfc @coveralls @ashleymeigh2