Skip to content

Fix windows-test failure due to NamedTemporaryFilePool#6417

Merged
c-bata merged 3 commits intooptuna:masterfrom
gen740:fix_tempfile_pool
Jan 26, 2026
Merged

Fix windows-test failure due to NamedTemporaryFilePool#6417
c-bata merged 3 commits intooptuna:masterfrom
gen740:fix_tempfile_pool

Conversation

@gen740
Copy link
Copy Markdown
Member

@gen740 gen740 commented Jan 16, 2026

Motivation

Sometimes, Windows-test failed due to a Permission Error (https://github.com/optuna/optuna/actions/runs/20156155033/job/57859104062?pr=6369).
This PR fixes this issue to make NamedTemporaryFilePool thread-safe.

Description of the changes

  • Make NamedTemporaryFilePool thread-safe.

@not522
Copy link
Copy Markdown
Member

not522 commented Jan 19, 2026

@c-bata Could you review this PR?

@c-bata c-bata added the CI Continuous integration. label Jan 19, 2026
@not522 not522 changed the title Fix windows-test failure due to NamedTemporaryFilePool Fix windows-test failure due to NamedTemporaryFilePool Jan 22, 2026
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM! I left brief comments, but it's fine to merge without addressing them.


def tempfile(self) -> IO[bytes] | IO[str]:
f = cast("IO[bytes] | IO[str]", tempfile.NamedTemporaryFile(delete=False, **self.kwargs))
self._file = f
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unrelated to this PR, but it might be good to assert that self._file is either None or a closed file here.

@not522 not522 removed their assignment Jan 23, 2026
gen740 and others added 2 commits January 23, 2026 10:12
Co-authored-by: Naoto Mizuno <naotomizuno@preferred.jp>
Co-authored-by: Naoto Mizuno <naotomizuno@preferred.jp>
Copy link
Copy Markdown
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM.

@c-bata c-bata added this to the v4.8.0 milestone Jan 26, 2026
@c-bata c-bata merged commit 6f35555 into optuna:master Jan 26, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants