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

[Feature] example_data for NonTensor spec #2698

Open
wants to merge 2 commits into
base: gh/vmoens/67/base
Choose a base branch
from

Conversation

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Jan 17, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/2698

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures, 7 Pending, 4 Unrelated Failures

As of commit 69c6122 with merge base 256a700 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@vmoens
Copy link
Contributor Author

vmoens commented Jan 17, 2025

@kurtamohler I think it would also be useful to have a spec that specified a set of available choices.
I was thinking to do that in NonTensor first (like NonTensor(choices["a", "b", "c"]), where rand() would pick one of the three values) but that seem restrictive (we'd need another one if you can select some tensor values).

One solution I'm contemplating is to have a Choice spec that works like this:

choice = Choice(
    torch.stack([
        NonTensor("a"),
        NonTensor("b"),
        NonTensor("c"),
    ])
)
choice.rand() # gives NonTensorData("a"), for instance

Using this you could do

choice = Choice(
    torch.stack([
        Bounded(0, 1),
        Bounded(5, 10),
    ])
)
choice.rand() # gives a tensor with value in [0, 5, 6, 7, 8, 9]

[ghstack-poisoned]
@kurtamohler
Copy link
Collaborator

kurtamohler commented Jan 17, 2025

Choice sounds like a useful spec. I could implement that if you'd like

I like this example_data feature--it partially addresses something I saw when writing Hash/UnaryTransform, which caused me to make this stipulation about None inputs:

fn (Callable): the function to use as the unary operation. If it accepts
a non-tensor input, it must also accept ``None``.

The only reason that stipulation exists is that when UnaryTransform calls spec.zero() to make a fake input to determine the output spec, if the spec has a NonTensor, the result for it is None. But after this PR, the example_data gets chosen instead of None.

Just a thought--maybe example_data could have a sensible default other than None for some dtypes? Like for str, it could be "" or even "example_data"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants