Skip to content

Fix for issue 3442 #3445

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix for issue 3442 #3445

wants to merge 7 commits into from

Conversation

JanVogelsang
Copy link
Contributor

This PR fixes issue #3442.

@JanVogelsang JanVogelsang requested a review from heplesser March 26, 2025 10:27
@JanVogelsang JanVogelsang self-assigned this Mar 26, 2025
@JanVogelsang JanVogelsang added T: Bug Wrong statements in the code or documentation S: Critical Needs to be addressed immediately labels Mar 26, 2025
@JanVogelsang JanVogelsang added this to the NEST 3.9 milestone Mar 26, 2025
@github-project-automation github-project-automation bot moved this to In progress in Kernel Mar 26, 2025
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

Looks good already. I have a few suggestions for further improvement. I also wonder if we should go "all in" in the CI and in our "everything" test also activate detailed and mpi-sync timer. That will slow the testsuite a little, but at least we test all. That would require adding something like "detailed_timers" and "mpi_sync_timer" to

- "openmp, mpi, python, gsl, ltdl, boost, hdf5, sionlib, libneurosim, optimize, warning, music"
and append the corresponding switches here:
-Dwith-music=${{ contains(matrix.use, 'music') && '$HOME/.cache/music.install' || 'OFF' }} \

@github-project-automation github-project-automation bot moved this from In progress to PRs pending approval in Kernel Mar 26, 2025
@heplesser heplesser requested a review from terhorstd March 26, 2025 12:35
@heplesser heplesser added S: High Should be handled next and removed S: Critical Needs to be addressed immediately labels Mar 26, 2025
@heplesser
Copy link
Contributor

I downgraded severity to "high" because it is a bug that only occurs under specific circumstances.

@JanVogelsang
Copy link
Contributor Author

I agree, adding detailed timers and the mpi-sync-timer to the "full" CI testcase makes sense. It shouldn't slow it down too much, so we should be fine.

@JanVogelsang JanVogelsang requested a review from heplesser March 27, 2025 11:01
@heplesser
Copy link
Contributor

@terhorstd Could you take a look at this one especially with a view to the changes in the build matrix?

@heplesser
Copy link
Contributor

@terhorstd Ping!

@JanVogelsang
Copy link
Contributor Author

@terhorstd You only need to review the changes in .github/workflows/nestbuildmatrix.yml, which should roughly take 12 seconds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Status: To do
Status: PRs pending approval
Development

Successfully merging this pull request may close these issues.

2 participants