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

Frequent timeouts for mac (3.11) workflow runs #1383

Closed
dweindl opened this issue May 2, 2024 · 3 comments · Fixed by #1388
Closed

Frequent timeouts for mac (3.11) workflow runs #1383

dweindl opened this issue May 2, 2024 · 3 comments · Fixed by #1388
Assignees
@PaulJonasJost
Labels
CI fixed but not released PEtab sampling Related to sampling

Comments

@dweindl
Copy link
Member

dweindl commented May 2, 2024

There are currently frequent timeouts for mac (3.11) workflow runs.

The by far slowest test is:
239.30s call test/sample/test_sample.py::test_thermodynamic_integration

Can this be reduced to <1min?
@dweindl dweindl added CI sampling Related to sampling labels May 2, 2024
@PaulJonasJost
Copy link
Collaborator

PaulJonasJost commented May 3, 2024 •
edited
Loading

will have a look 👍🏼

@PaulJonasJost PaulJonasJost self-assigned this May 3, 2024
dweindl added a commit to dweindl/pyPESTO that referenced this issue May 3, 2024
@dweindl
GHA: use macos-12 runner
664e706 
Should prevent unclear timeouts ICB-DCM#1383.
@dweindl dweindl mentioned this issue May 3, 2024
Merged
dweindl added a commit that referenced this issue May 3, 2024
@dweindl
GHA: use macos-12 runner (#1386)
4f778cc 
Prevents unclear timeouts that occur with `macos-14-arm64` (see #1383).
dweindl added a commit to dweindl/pyPESTO that referenced this issue May 3, 2024
@dweindl
Fix pypesto.sample.geweke_test.spectrum for nfft<=3
149bbe0 
For `nfft<=3`, this function computed `nk` by converting infinity to int64.

For the last [4 years](ICB-DCM@fdbacb4), this seemed to not cause any trouble,
because `np.float64("inf").astype(int)` yielded -9223372036854775808 and the loop did not run.
However, with the current macos-14 GitHub runner, this yields 9223372036854775807, and accordingly, to rather long computation times (this is what caused ICB-DCM#1383).

I am not really sure what caused this change. macos-12 vs macos-14, arm64 vs x86, ...? all should be IEEE 754-compliant. Maybe some different integer type? 🤷‍♂️

With this change macos-14 yields the same results as the other runners.
However, I am not familar with computing power spectral density and I don't know if this is indeed what should happen.
It would be great if somebody more familiar with that topic could double-check.
(Also, can't we use [scipy.signal.welch](https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.welch.html#scipy.signal.welch) here?)
@dweindl dweindl mentioned this issue May 3, 2024
Merged
@dweindl
Copy link
Member Author

dweindl commented May 4, 2024

The by far slowest test is:
239.30s call test/sample/test_sample.py::test_thermodynamic_integration

I take that back. This, as well as the timeouts in general, was a consequence of the issue addressed in #1388.
After fixing that, the duration is okayish:
35.52s call test/sample/test_sample.py::test_thermodynamic_integration

dweindl added a commit that referenced this issue May 4, 2024
@dweindl
Fix pypesto.sample.geweke_test.spectrum for nfft<=3 (#1388)
cacd525 
For `nfft<=3`, this function computed `k` by converting infinity to int64.

For the last [4 years](fdbacb4), this seemed to not cause any trouble, because `np.float64("inf").astype(int)` yielded -9223372036854775808 and the loop did not run. However, with the current `macos-14` GitHub runner, this yields 9223372036854775807, and accordingly, rather long computation times (this is what caused #1383).

I am not really sure what caused this change. macos-12 vs macos-14, arm64 vs x86, ...? all should be IEEE 754-compliant. Maybe some different integer type? 🤷‍♂️

With this change `macos-14` runners yield the same results as the other runners. However, I am not familiar with computing power spectral density and I don't know if this is indeed what should happen. It would be great if somebody more familiar with that topic could double-check. (Also, can't we use [scipy.signal.welch](https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.welch.html#scipy.signal.welch) here?)
@dweindl dweindl linked a pull request May 5, 2024 that will close this issue
Fix pypesto.sample.geweke_test.spectrum for nfft<=3 #1388
Merged
@PaulJonasJost
Copy link
Collaborator

I think this should be fixed right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PaulJonasJost PaulJonasJost

Labels
CI fixed but not released PEtab sampling Related to sampling
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix pypesto.sample.geweke_test.spectrum for nfft<=3 dweindl/pyPESTO
2 participants
@dweindl @PaulJonasJost