-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix captured random state in compile #3828
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // Copyright © 2026 Apple Inc. | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <nanobind/nanobind.h> | ||
|
|
||
| #include "mlx/array.h" | ||
|
|
||
| namespace mx = mlx::core; | ||
| namespace nb = nanobind; | ||
|
|
||
| // The process-global `mx.random.state` sentinel. | ||
| nb::object random_state_sentinel(); | ||
|
|
||
| // True if `obj` is the RNG state sentinel. | ||
| bool is_random_state(nb::handle obj); | ||
|
|
||
| // Read/write the calling thread's current PRNG key. | ||
| mx::array random_state_key(); | ||
| void set_random_state_key(const mx::array& key); |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||
| // Copyright © 2023-2024 Apple Inc. | ||||||||||
|
|
||||||||||
| #include "python/src/trees.h" | ||||||||||
| #include "python/src/random.h" | ||||||||||
|
|
||||||||||
| template <typename T, typename U, typename V> | ||||||||||
| void validate_subtrees(const std::vector<nb::object>& subtrees) { | ||||||||||
|
|
@@ -153,7 +154,10 @@ void tree_visit( | |||||||||
| void tree_visit(nb::handle tree, std::function<void(nb::handle)> visitor) { | ||||||||||
| std::function<void(nb::handle)> recurse; | ||||||||||
| recurse = [&](nb::handle subtree) { | ||||||||||
| if (nb::isinstance<nb::list>(subtree) || | ||||||||||
| if (is_random_state(subtree)) { | ||||||||||
| visitor(nb::cast(random_state_key())); | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can do this in a less intrusive way by replacing Lines 560 to 563 in 4367c73
with if (!captured_inputs.is_none()) {
trace_captures.push_back(random_state_sentinel());
flat_in_captures.push_back(random_state_key());
tree_replace(captured_inputs, trace_captures, flat_in_captures);
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is the random state can be in any place in the captured inputs. As a result we have two options
Option 1 has the overhead of 2 extra tree walks which for say 100k nodes (like a big transformer or sth) can be non-negligible. Option 2 is intrusive and ugly.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, we can probably add a callback to |
||||||||||
| } else if ( | ||||||||||
| nb::isinstance<nb::list>(subtree) || | ||||||||||
| nb::isinstance<nb::tuple>(subtree)) { | ||||||||||
| for (auto item : subtree) { | ||||||||||
| recurse(item); | ||||||||||
|
|
@@ -175,7 +179,12 @@ void tree_visit_update( | |||||||||
| std::function<nb::object(nb::handle)> visitor) { | ||||||||||
| std::function<nb::object(nb::handle)> recurse; | ||||||||||
| recurse = [&](nb::handle subtree) { | ||||||||||
| if (nb::isinstance<nb::list>(subtree)) { | ||||||||||
| if (is_random_state(subtree)) { | ||||||||||
| // Read/write the calling thread's key; keep the sentinel in the tree. | ||||||||||
| set_random_state_key( | ||||||||||
| nb::cast<mx::array>(visitor(nb::cast(random_state_key())))); | ||||||||||
| return nb::cast<nb::object>(subtree); | ||||||||||
| } else if (nb::isinstance<nb::list>(subtree)) { | ||||||||||
| auto l = nb::cast<nb::list>(subtree); | ||||||||||
| for (int i = 0; i < l.size(); ++i) { | ||||||||||
| l[i] = recurse(l[i]); | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.