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

Allow Proxy creation without a set TraceCtx #1598

Draft
wants to merge 5 commits into
base: proxy-update2
Choose a base branch
from

Conversation

IvanYashchuk
Copy link
Collaborator

Now creating TensorProxies is as simple as in PyTorch:

In [1]: from thunder.core.proxies import TensorProxy

In [2]: from thunder.core.dtypes import float32

In [3]: from thunder.core.devices import cpu

In [4]: TensorProxy(shape=(1,), device=cpu, dtype=float32)
Out[4]: <TensorProxy(name="None", dtype=thunder.dtypes.float32, shape=(1,))>

Base PR: #1597.
Fixes #1593.

@mruberry
Copy link
Collaborator

I'm surprised this appears to be causing CI failures! I wonder what's going on.

Fixing #1593 sounds great, and this looks like a surgical way to support the creation of tensor proxies outside of a trace. What can we actually do with these tensor proxies — just call meta functions directly on them? That alone sounds interesting.

Could we make the creation of traceless tensor proxies more explicit — at least for now — like by requiring a special function be called? The function could pass a new option kwarg to the existing functions (like Proxy creation and name origination), like traceless=True, which they could query to change their behavior. The error when creating a proxy outside a trace without setting traceless could be more explicit then, too, preventing accidents.

In the future (or this PR), we could also imagine that name generation is still automatic when traceless=True is set. We could have a traceless state that keeps a counter, for example, and returns names like t0, t1, ...

@t-vi
Copy link
Collaborator

t-vi commented Dec 30, 2024

This is exactly the opposite direction of where we should move. Traces need to own proxies more, not less.
We have had a ton of misery from name clashes in 2024 and we don't need to increase that going forward.

@mruberry
Copy link
Collaborator

This is exactly the opposite direction of where we should move. Traces need to own proxies more, not less. We have had a ton of misery from name clashes in 2024 and we don't need to increase that going forward.

I don't think these ideas have to be in conflict. The traceless proxies couldn't be used in any trace (because they're not owned by any trace), so they shouldn't interoperate with proxies owned by a trace

@IvanYashchuk
Copy link
Collaborator Author

I'm surprised this appears to be causing CI failures! I wonder what's going on.

All the failures are from the base branch (#1596) and are now resolved. The errors were due to the reliance of jit_ext.py file on the assumption that a new name would be generated if the requested name is used (fixed now).

@IvanYashchuk
Copy link
Collaborator Author

What can we actually do with these tensor proxies — just call meta functions directly on them?

Yes, call meta functions, but only simple ones that do not require an active trace (ones that do not call other symbols). It's useful for development, exploration, and debugging. Less concepts to explain and keep in mind to a new developer when beginning to explain the internals of Thunder.

In the future (or this PR), we could also imagine that name generation is still automatic when traceless=True is set. We could have a traceless state that keeps a counter, for example, and returns names like t0, t1, ...

Yes, we used to have a global counter in the early versions. Actual names are needed when a bound symbol is constructed to generate a Python line of code.

Could we make the creation of traceless tensor proxies more explicit — at least for now — like by requiring a special function be called? The function could pass a new option kwarg to the existing functions (like Proxy creation and name origination), like traceless=True, which they could query to change their behavior. The error when creating a proxy outside a trace without setting traceless could be more explicit then, too, preventing accidents.

Sure, we can do that. My intention is to decrease cognitive friction for someone coming from other array frameworks. Setting traceless=True means someone needs to understand (and we need to explain) what a "traceful" proxy is.

@t-vi
Copy link
Collaborator

t-vi commented Jan 7, 2025

Can we make this a utility function "traceless_tensorproxy" or so rather than baking it into the proxy main path, please?

I would prefer not to have proxies that will be added to traces next being created outside the trace context by accident.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants