Performance optimization for `test_study.py` by removing redundancy by himkt · Pull Request #6120 · optuna/optuna

himkt · 2025-05-31T07:57:40Z

timeout is unnecessarily long for the very simple objective function abb7817
- objectives in test_study.py just return constant value, which finishes to run very shortly
n_trials is unnecessarily large for scenarios where it just needs multiple trials abb7817
Using concurrent.futures.ProcessPoolExecutor instead of multiprocessing.Pool 2c0bb80
Introduce a helper function to check if stderr prosibly contain progressbar c252476

Before

=========== 595 passed, 5 skipped, 91 warnings in 131.25s (0:02:11) ============

After

============ 595 passed, 5 skipped, 91 warnings in 75.25s (0:01:15) ============

Motivation

Description of the changes

himkt · 2025-05-31T07:59:40Z

tests/study_tests/test_study.py

+        with ProcessPoolExecutor() as pool:
+            # Test normal behaviour.
+            pool.submit(_process_tell, study=study, trial=trial0, values=1.2).result()
+            assert len(study.trials) == 1
+            assert study.best_trial.state == TrialState.COMPLETE
+            assert study.best_value == 1.2
+
+            # Test study.tell using trial number.
+            trial = study.ask()
+            pool.submit(_process_tell, study=study, trial=trial.number, values=1.5).result()
+            assert len(study.trials) == 2
+            assert study.best_trial.state == TrialState.COMPLETE
+            assert study.best_value == 1.2
+
+            # Should fail because the trial0 is already finished.
+            with pytest.raises(ValueError):
+                fut = pool.submit(_process_tell, study=study, trial=trial0, values=1.2)
+                fut.result()


Using ProcessPoolExecutor and submit is consistently faster than multiprocessing.Pool and starmap.

Before

> git checkout abb781787 HEAD is now at abb781787 chore: introduce consts for minimal values of n_trials and timeout > uv run python -m pytest tests/study_tests/test_study.py -k test_tell_from_another_process =============================================================================== test session starts =============================================================================== platform darwin -- Python 3.12.0, pytest-8.3.5, pluggy-1.6.0 rootdir: /Users/himkt/work/github.com/himkt/optuna configfile: pyproject.toml plugins: profiling-1.8.1 collected 600 items / 599 deselected / 1 selected tests/study_tests/test_study.py . [100%] ======================================================================== 1 passed, 599 deselected in 4.43s ========================================================================

After

> git checkout 2c0bb80c4 Previous HEAD position was abb781787 chore: introduce consts for minimal values of n_trials and timeout HEAD is now at 2c0bb80c4 chore: use ProcessPoolExecutor instead of multiprocessing.Pool >uv run python -m pytest tests/study_tests/test_study.py -k test_tell_from_another_process =============================================================================== test session starts =============================================================================== platform darwin -- Python 3.12.0, pytest-8.3.5, pluggy-1.6.0 rootdir: /Users/himkt/work/github.com/himkt/optuna configfile: pyproject.toml plugins: profiling-1.8.1 collected 600 items / 599 deselected / 1 selected tests/study_tests/test_study.py . [100%] ======================================================================== 1 passed, 599 deselected in 2.26s ========================================================================

Copilot

Pull Request Overview

This PR reduces the number of trials and the timeout values for trivial objective functions to speed up test execution and switches from multiprocessing.Pool to using ProcessPoolExecutor. The changes update several tests to use defined constants and adjust assertions accordingly.

Reduced n_trials from 10 to a constant value for faster tests.
Updated timeout values for short-running objectives.
Switched from multiprocessing.Pool to ProcessPoolExecutor for cross-process communication.

Comments suppressed due to low confidence (1)

tests/study_tests/test_study.py:50

The constant 'NUM_MINMAL_TRIALS' appears to have a typo. Consider renaming it to 'NUM_MINIMAL_TRIALS' for clarity.

NUM_MINMAL_TRIALS = 2

tests/study_tests/test_study.py

Copilot

Pull Request Overview

This PR reduces test execution time by introducing minimal trial/time constants, switches to ProcessPoolExecutor for cross-process testing, and centralizes progress bar validation.

Define NUM_MINMAL_TRIALS and MINIMUM_TIMEOUT_SEC and replace hard-coded values
Add ProcessPoolExecutor for testing tell() across processes
Introduce check_progressbar helper and refactor progress bar assertions

Comments suppressed due to low confidence (2)

tests/study_tests/test_study.py:50

There’s a typo in the constant name NUM_MINMAL_TRIALS; consider renaming it to NUM_MINIMAL_TRIALS for clarity.

NUM_MINMAL_TRIALS = 2

tests/study_tests/test_study.py:8

The multiprocessing import is no longer used after switching to ProcessPoolExecutor; it can be removed to clean up unused imports.

import multiprocessing

Copilot · 2025-05-31T08:34:15Z

tests/study_tests/test_study.py

    _, err = capsys.readouterr()

-    assert "Best trial: 0" in err
+    assert "Best trial: 0"


This assertion always passes since it doesn’t check the error output. It should be assert "Best trial: 0" in err.

Suggested change

assert "Best trial: 0"

assert "Best trial: 0" in err

contramundum53 · 2025-05-31T08:36:57Z

tests/study_tests/test_study.py


 CallbackFuncType = TypingCallable[[Study, FrozenTrial], None]

+NUM_MINMAL_TRIALS = 2


Suggested change

NUM_MINMAL_TRIALS = 2

NUM_MINIMAL_TRIALS = 2

Copilot is actually right and there's a typo.

fixed in 41664ac. 🙏

contramundum53 · 2025-05-31T08:37:46Z

tests/study_tests/test_study.py

+from concurrent.futures import ProcessPoolExecutor
 from concurrent.futures import ThreadPoolExecutor
 import copy
 import multiprocessing


Suggested change

import multiprocessing

multiprocessing is no longer needed (again, by copilot)

Actually, multiprocessing is used in another test... 😄

optuna/tests/study_tests/test_study.py

Line 219 in daeef68

n_jobs_actual = n_jobs if n_jobs != -1 else multiprocessing.cpu_count()

…stable with 2 😇

…are not provided

himkt · 2025-06-02T06:13:08Z

The PR is now ready for review again.

himkt · 2025-06-02T06:17:24Z

tests/study_tests/test_study.py

        sampler = study.sampler
        with patch.object(sampler, "reseed_rng", wraps=sampler.reseed_rng) as mock_object:
-            study.optimize(f, n_trials=1, n_jobs=2)
+            study.optimize(f, n_trials=1, n_jobs=n_jobs)


I'm not confident but I think test_optimize_with_reseeding unexpectedly runs tests with the fixed configuration (n_jobs=2) even with parametrization.

not522 · 2025-06-02T10:00:59Z

Could you check the CI fail?

himkt · 2025-06-02T11:45:58Z

Sorry I just thought I fixed all failures... resolved in 79b9807.

contramundum53

LGTM.

himkt added 2 commits May 31, 2025 16:52

chore: introduce consts for minimal values of n_trials and timeout

abb7817

chore: use ProcessPoolExecutor instead of multiprocessing.Pool

2c0bb80

himkt self-assigned this May 31, 2025

himkt commented May 31, 2025

View reviewed changes

himkt added 2 commits May 31, 2025 17:14

fix: ./formats.sh

895de1c

chore: introduce helper to check if output prosibly contains progressbar

c252476

himkt requested a review from Copilot May 31, 2025 08:25

Copilot AI reviewed May 31, 2025

View reviewed changes

tests/study_tests/test_study.py Outdated Show resolved Hide resolved

y0z added the sprint-20250531 PR from the online sprint event May 31, 2025. label May 31, 2025

fix: test_optimize_without_progbar should not see progressbar

daeef68

himkt requested a review from Copilot May 31, 2025 08:33

himkt changed the title ~~(wip)~~ Performance optimization for test_study.py by removing redundancy May 31, 2025

Copilot AI reviewed May 31, 2025

View reviewed changes

contramundum53 reviewed May 31, 2025

View reviewed changes

fix: assertion was missed

dfeeffe

contramundum53 reviewed May 31, 2025

View reviewed changes

fix: type MINMAL -> MINIMAL

41664ac

himkt marked this pull request as draft May 31, 2025 14:18

himkt added 7 commits June 1, 2025 23:22

fix: is_shown=False for without_progbar

40f796a

chore: increase NUM_MINIMAL_TRIALS since progbar rendering becomes un…

4e8918a

…stable with 2 😇

fix: progressbar should not be rendered when n_jobs>1

83d6521

fix: ./formats.sh

c67836a

fix: iterations per sec should not be appeared if n_trials is omitted

bc28ee7

fix: progressbar should not be rendered when both n_jobs and timeout …

290c3dc

…are not provided

chore: revert 4e8918a

ecf1c26

himkt marked this pull request as ready for review June 2, 2025 06:12

himkt commented Jun 2, 2025

View reviewed changes

fix: trial.number starts from zero

79b9807

contramundum53 approved these changes Jun 4, 2025

View reviewed changes

contramundum53 merged commit ca18359 into optuna:master Jun 4, 2025
14 checks passed

y0z added this to the v4.4.0 milestone Jun 4, 2025

HideakiImamura added the test Unit test. label Jun 12, 2025


		CallbackFuncType = TypingCallable[[Study, FrozenTrial], None]

		NUM_MINMAL_TRIALS = 2

Uh oh!

Conversation

himkt commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Motivation

Description of the changes

Uh oh!

himkt May 31, 2025

Choose a reason for hiding this comment

Before

After

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 31, 2025

Choose a reason for hiding this comment

Uh oh!

himkt May 31, 2025

Choose a reason for hiding this comment

Uh oh!

contramundum53 May 31, 2025

Choose a reason for hiding this comment

Uh oh!

himkt May 31, 2025

Choose a reason for hiding this comment

Uh oh!

contramundum53 May 31, 2025

Choose a reason for hiding this comment

Uh oh!

himkt May 31, 2025

Choose a reason for hiding this comment

Uh oh!

himkt commented Jun 2, 2025

Uh oh!

himkt Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

not522 commented Jun 2, 2025

Uh oh!

himkt commented Jun 2, 2025

Uh oh!

contramundum53 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

himkt commented May 31, 2025 •

edited

Loading