Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stackless, redux #38

Merged
merged 5 commits into from
Jan 27, 2025
Merged

Conversation

nstarman
Copy link
Contributor

@nstarman nstarman commented Jan 17, 2025

Hi @patrick-kidger @mattjj!
I cherry-picked @mattjj's commits, fixed the CI, and made the suggested refactor.
I'm happy to push these fixes as a PR to @mattjj's branch instead of this PR.

The changes in jax also require python 3.10+

mattjj and others added 3 commits January 17, 2025 17:00
`pytest tests` passes with these changes
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman nstarman force-pushed the stackless-redux branch 5 times, most recently from 4fbc627 to c0c7530 Compare January 17, 2025 22:58
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@patrick-kidger
Copy link
Owner

Nice! Thank you for putting this together.

Do you have an idea how to tackle #37 (comment) ?

dynamic = jtu.tree_map(
ft.partial(_wrap_tracer, trace),
dynamic,
is_leaf=_is_value,
)
fn, args, kwargs = eqx.combine(dynamic, static)
out = fn(*args, **kwargs)
with core.set_current_trace(trace):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrick-kidger @mattjj does this look like the place to check that Quax values are wrapped into tracers?
#37 (comment)

Copy link
Contributor Author

@nstarman nstarman Jan 21, 2025

Choose a reason for hiding this comment

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

I can do some testing, but an informed starting point would be great!
There doesn't appear to be an obvious place in _QuaxTrace anymore, so I was looking at here, but it's also not obviously the right place for checking the tracer-wrapping.

@nstarman
Copy link
Contributor Author

nstarman commented Jan 23, 2025

@patrick-kidger
Error raised. It might be working already?!

(I bumped the version to show it was picking up the updated code)

@patrick-kidger patrick-kidger merged commit 605e104 into patrick-kidger:main Jan 27, 2025
3 checks passed
@patrick-kidger
Copy link
Owner

Ah, interesting. In that case I think I'm happy to merge this as-is. This is all still pretty new so we'll just have to see how it goes, I suppose!

If you'd like to bump the version number then I'm happy to do a new release. :) Thank you for taking over this PR!

@nstarman nstarman deleted the stackless-redux branch January 28, 2025 02:17
@nstarman
Copy link
Contributor Author

If you'd like to bump the version number then I'm happy to do a new release. :) Thank you for taking over this PR!

I'll do that!
I'll also add a test for the above test case, which I forgot to do in this PR.

@nstarman nstarman mentioned this pull request Jan 28, 2025
@patrick-kidger patrick-kidger mentioned this pull request Feb 1, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants