Skip to content

Actually use scheduler in reverse rules#251

Merged
lkdvos merged 1 commit intomasterfrom
reverse-scheduler
Sep 3, 2025
Merged

Actually use scheduler in reverse rules#251
lkdvos merged 1 commit intomasterfrom
reverse-scheduler

Conversation

@lkdvos
Copy link
Member

@lkdvos lkdvos commented Sep 2, 2025

This is a small change that ensures that we are actually using the correct scheduler in the reverse rules. I don't really understand why or how this is happening, but I noticed that the current way the rrule was implemented seems to intercept the callstack before the default keyword gets filled in, therefore always using the default scheduler, even if set to :serial.

@lkdvos lkdvos requested a review from pbrehmer September 2, 2025 20:18
@codecov
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/utility/diffable_threads.jl 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@pbrehmer pbrehmer 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 fixing that! I also don't get why the kwargs of dtmap are not being passed onto its rrule. However, I vaguely remember having problems with this before and IIRC I had to explicitly pass the kwargs to the forward function call, i.e. the default kwargs wouldn't be used somehow.

Anyways, now that the schedulers of the forward and backward pass are essentially independent, what would you think of having two separate forward and backward schedulers? I bring this up since I was worried that multi threading the backward pass might be quite inefficient in some cases and then it would be useful to set just the backwards scheduler to :serial. In any case that would be another PR, so this is ready to go for me.

@lkdvos lkdvos merged commit 24c6730 into master Sep 3, 2025
45 checks passed
@lkdvos lkdvos deleted the reverse-scheduler branch September 3, 2025 11:05
@lkdvos
Copy link
Member Author

lkdvos commented Sep 3, 2025

I’m definitely okay with having both separately, I’ll see what I can do

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