perf(tidy3d): FXC-3721 Speed up test suite #2991
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
dd55b29 to
0d5292d
Compare
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
2 times, most recently
from
700ecb3 to
693676d
Compare
Contributor
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
Collaborator
There was a problem hiding this comment.
I'm wondering a bit what benefit this provides over just running pytest --durations > times.txt?
Comment on lines
+185
to
+190
| N_tests = 2 # may be increased temporarily, makes it slow for routine tests | ||
| max_structures = np.log10(2) # may be increased temporarily, makes it slow for routine tests | ||
|
|
||
| # adjust as needed, keeping small to speed tests up | ||
| num_structures = np.logspace(0, 2, N_tests).astype(int) | ||
| num_structures = np.logspace(0, max_structures, N_tests).astype(int) |
Collaborator
There was a problem hiding this comment.
i think this turns the test in this file into a smoke-test essentially. i believe the purpose there was to catch regressions related to validation time where the serialization scales poorly as structure count grows. now we basically just assert that both cases work. but to be fair the old test didnt actually assert any regressions so i'm not really sure what makes sense here, maybe @momchil-flex has an opinion?
Contributor
Author
There was a problem hiding this comment.
made for now a "perf" marker and deselected for regular tests
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
5bbca11 to
3f326b4
Compare
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
3f326b4 to
9c5ff18
Compare
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
9c5ff18 to
8e7ff17
Compare
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
9bef6f0 to
f310e32
Compare
marcorudolphflex
force-pushed
the
FXC-3721-speed-up-test-suite
branch
from
f310e32 to
8c9050b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As our test suite does have a quite long runtime, this PR addresses the identification of the most time-intensive modules/functions/runs and speeds up the most critical ones.
profile_pytest.pyin scripts to analyze the most time-consuming modules/functions/runs (see report below)pytest.mark.slowfor the most time-consuming tests. May be used later for skipping for faster test modes.test_autograd.pyas the most critical module (10 min raw runtime!) -> cut it down by roughly the half. Made sure these tests run in the beginning such that we avoid non-fully-parallelized works are on them at the end.I'm open for more inputs how to speedup the tests.
Analysis before PR:
Analysis after PR
Greptile Overview
Greptile Summary
This PR successfully optimizes the test suite runtime by ~30% (from 195s to 137s total, with
test_autograd.pyreduced from 635s to 330s) through strategic refactoring:Major Changes:
profile_pytest.pyscript to identify and track slow tests@pytest.mark.slowand@pytest.mark.perfmarkers for better test categorizationpytest-orderdependency to run heavy autograd tests first (avoiding non-parallelized tail execution)MAX_NUM_MEDIUMSconstant from production values to 3 in testsTrade-offs:
The optimizations reduce some test coverage (e.g.,
test_multi_frequency_equivalencenow only tests one structure type instead of 12, async tests run 2 structure/monitor combinations instead of 16×2=32), but this is reasonable since the core functionality is validated through equivalence checks and the reduced coverage focuses on structural diversity rather than functional correctness.Confidence Score: 4/5
Important Files Changed
File Analysis
perf,slow) - proper pytest configuration updates@pytest.mark.perf@pytest.mark.slowSequence Diagram
sequenceDiagram participant Dev as Developer participant Script as profile_pytest.py participant PyTest as pytest participant Tests as Test Suite Dev->>Script: Run profiling script Script->>PyTest: Execute with --durations flag PyTest->>Tests: Run test collection Note over Tests: Tests run with optimizations:<br/>- Reduced parameterizations<br/>- Batched async operations<br/>- Marked slow tests<br/>- Reduced iteration counts Tests-->>PyTest: Test results + timing data PyTest-->>Script: Duration statistics Script->>Script: Parse & aggregate durations Script->>Script: Generate report by file & test Script-->>Dev: Performance report (pytest_profile_stats.txt) Note over Dev: 30% faster test suite<br/>635s → 330s for autograd tests