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

Update for Brian 2.6 #314

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Update for Brian 2.6 #314

merged 10 commits into from
Mar 26, 2024

Conversation

mstimberg
Copy link
Member

This PR should move Brian2CUDA to compatibility with Brian 2.6, in particular with its useful new run_args feature.

Two remarks:

  1. The C++ code for the run_args feature is an ugly mix of compile-time and run-time code, hopefully we can refactor this nicely in the future (it shouldn't have to be duplicated between Brian2 and Brian2CUDA in the first place).
  2. I made it easier to use Brian2's logging feature for external packages like Brian2CUDA, but unfortunately there was a little bug (already fixed, but not in the 2.6.0 release, see Fix catch_logs behaviour for 3rd-party modules brian2#1519). Brian tests were meant to ignore log messages from packages not coming from brian2, but it erroneously included all package names starting with brian2, so brian2cuda log messages were still counted...

Copy link
Member

@denisalevi denisalevi left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this @mstimberg ! Looks good to me. I like the catch_logs updates, I struggled a bit to get it working with brian2cuda and it needed occasional test updates, so glad this seems to be fixed. But does the bug you referenced mean that tests are currently failing and will pass for the next Brian2 release? If so, should we add that as expected to fail to the test suite?

I didn't follow the run_args implementation in detail, but I'm assuming its mostly copy-paste since its running all in host code, right?

And are you running the test suite locally with GPU access? Or should I do that (I would have to set things up first since our cluster infrastructure has changed and I haven't used it since).

Approving. Feel free to merge if tests have run.

@mstimberg
Copy link
Member Author

Hi. So there are several things related to logs that have been fixed (or were supposed to) :

  1. You do not have to invent names that start with brian2 to make use of Brian's infrastructure. E.g., previously brian2cuda.cuda_generator displayed as brian2.codegen.generators.cuda_generator, which didn't correspond to an actual package. This is no longer necessary.
  2. In Brian2CUDA's tests, you can ask catch_logs to only consider logs from brian2cuda, so we don't have to worry about Brian2 adding/removing warnings.
  3. In Brian2's tests, only logs from brian2 are considered, so having a warning in Brian2CUDA doesn't break tests

Points 1 and 2 work fine, point 3 is unfortunately broken in 2.6.0. (but already fixed in master). For now, this simply means that we need to keep the workarounds that were already in place, i.e. the ignore_brian2_tests feature in brian2cuda.tests.__init__, and the duplicated test_neurongroup.py::test_semantics_floor_division test.

I didn't follow the run_args implementation in detail, but I'm assuming its mostly copy-paste since its running all in host code, right?

Yes, it's ugly, but it is exactly the same ugliness as in C++ standalone ;)

And are you running the test suite locally with GPU access? Or should I do that (I would have to set things up first since our cluster infrastructure has changed and I haven't used it since).

I've been running the tests on my laptop with a GPU – not fast, but good enough for now.

A bit of a silly issue, but 2.6.0 may render 0 and 1 as false and true, breaking a check in the rng code.

See brian-team/brian2#1520
@mstimberg
Copy link
Member Author

I forgot to fix a test (for a silly thing, see brian-team/brian2#1520), but now all tests pass on my machine 👍 This also means it is time to do a new release! If I am not mistaken, this simply means creating a new tag (v1.0a4?), so this should be straightforward.

@mstimberg mstimberg merged commit 5685699 into master Mar 26, 2024
2 checks passed
@mstimberg mstimberg deleted the update_for_brian_2.6 branch March 26, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants