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

feat(specs): make environment specs managed attributes #220

Merged
merged 19 commits into from
Mar 21, 2024

Conversation

aar65537
Copy link
Contributor

@aar65537 aar65537 commented Jan 24, 2024

closes #98

  • Implements private _*_spec properties on the Environment class. The properties are accessed through *_spec managed attributes implemented with the @property decorator. Properties are initialized with _make_*_spec() methods during Environment initialization.
  • Implements generic ActionSpec type on the Environment class in order to statically type the action_spec attribute.
  • Updates all environments to conform with changes to Environment class.
  • Implements unit tests to ensure Environment specs can be accessed from a jitted function.
  • Changes all references to *_spec() method to *_spec attribute.

Edit:
now also closes #221

  • Implements generic Observation type on Environment
  • Implements generic types on Wrapper classes, so that Wrappers will inherit type hints from the Environments they wrap

Copy link
Collaborator

@clement-bonnet clement-bonnet left a comment

Choose a reason for hiding this comment

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

Thank you @aar65537 for the contribution, it is a great improvement to the library! I just have one comment regarding the ActionSpec which I think we should not have.
Please let me know what you think!

jumanji/env.py Outdated Show resolved Hide resolved
jumanji/env.py Show resolved Hide resolved
@sash-a
Copy link
Collaborator

sash-a commented Mar 7, 2024

Sorry for the radio silence, I still want to get this in! What do you think about using cached_properties instead of properties as the spec should never change?

@aar65537
Copy link
Contributor Author

aar65537 commented Mar 8, 2024

I think cached_properties should work. The only issue I foresee is you might have to access the properties in the __init__ method to ensure the arrays are initialized outside of a jitted function.

@sash-a
Copy link
Collaborator

sash-a commented Mar 11, 2024

I think cached_properties should work. The only issue I foresee is you might have to access the properties in the __init__ method to ensure the arrays are initialized outside of a jitted function.

True, but we know what the cached properties are, so we could call them in the base env classes init?

@sash-a
Copy link
Collaborator

sash-a commented Mar 11, 2024

Also just a heads up, we're making a push to get sliding tile puzzle and flatpack envs merged before the ICLR camera ready deadline (about a week). Once this is done we'll get to this PR 😄

@aar65537
Copy link
Contributor Author

Thanks for the heads up. I changed all of the environments to use cached_property. I think it is more succinct than my original implementation. Thank you for the suggestion.

sash-a
sash-a previously approved these changes Mar 20, 2024
Copy link
Collaborator

@sash-a sash-a 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 this @aar65537, really nice change 😄

Copy link
Collaborator

@clement-bonnet clement-bonnet left a comment

Choose a reason for hiding this comment

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

All good to me! Except that we are missing changes for at least SlidingTilePuzzle since we merged it. Could we possibly port these specs changes to that 22nd environment as well, please?

@aar65537
Copy link
Contributor Author

I've implemented the changes for the SlidingTilePuzzle and cleaned up the last references to action_spec() in the docs. That should be all the environments.

@clement-bonnet clement-bonnet merged commit 1f38fe6 into instadeepai:main Mar 21, 2024
3 checks passed
@aar65537 aar65537 deleted the 98-spec-props branch March 25, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants