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

pytest: Temporary Working Directory #424

Merged
merged 4 commits into from
Aug 30, 2023
Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Aug 28, 2023

Tests can generate temporary output and directories, which might clash and/or need cleaning between tests.
This changes the current working directory to a unique directory per test.

Seen as:
AMReX counts seconds passed (and factions thereof) since initialization. For subsequent tests, this can collide.
Also, the UniqueStrings in AMReX' UtilCreateCleanDirectory is pretty coarse in MPI contexts:
https://github.com/AMReX-Codes/amrex/blob/23.08/Src/Base/AMReX_Utility.cpp#L186-L199

If the target exist, the behavior of std::rename is implementation dependent. We see crashes of this on macOS.
https://en.cppreference.com/w/cpp/io/c/rename

@ax3l ax3l added component: third party Changes in ImpactX that reflect a change in a third-party library workaround component: tests examples, tests and benchmarks labels Aug 28, 2023
@ax3l ax3l requested a review from WeiqunZhang August 28, 2023 18:59
Simetimes our tests are so quick that they
finish below 1second. That does not work with
`UniqueStrings` in AMReX' `UtilCreateCleanDirectory`,
which can only do one name per second:
https://github.com/AMReX-Codes/amrex/blob/23.08/Src/Base/AMReX_Utility.cpp#L186-L199

If the target exist, the behavior of `std::rename` is
implementation dependent. We see crashes of this
on macOS.
https://en.cppreference.com/w/cpp/io/c/rename
@ax3l
Copy link
Member Author

ax3l commented Aug 28, 2023

Update: the names are odd
this is what I see on macOS CI:

amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.4000000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.0000000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.4000000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.6400000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.5300000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.1400000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.2300000
amrex::UtilCreateCleanDirectory():  diags exists.  Renaming to:  diags.old.7600000

this does not seem right

Avoid clean-up needs by using a cwd that is unique per test.
@ax3l ax3l changed the title Fix: pytest Wait 1 Sec pytest: Temporary Working Directory Aug 28, 2023
@ax3l ax3l added bug Something isn't working bug: affects latest release Bug also exists in latest release version labels Aug 28, 2023
ax3l pushed a commit to AMReX-Codes/amrex that referenced this pull request Aug 28, 2023
# Summary

Previously UniqueString used MPI_Wtime, which could have a very low
resolution (1e-5s) on some machines. In this PR, we switch to use
amrex::second(), which uses the highest resolution steady clock (usually
1.e-9s). We have also changed a hardwired number according to resolution
of the clock so that the digits are not wasted on trailing zeros.

## Additional background

ECP-WarpX/impactx#424

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
tests/python/conftest.py Outdated Show resolved Hide resolved
@ax3l ax3l merged commit 4a52d2f into ECP-WarpX:development Aug 30, 2023
13 checks passed
@ax3l ax3l deleted the pytest-sleep-1s branch August 30, 2023 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working component: tests examples, tests and benchmarks component: third party Changes in ImpactX that reflect a change in a third-party library workaround
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant