Skip to content

Conversation

@nicolas-dufour
Copy link
Contributor

@nicolas-dufour nicolas-dufour commented Sep 7, 2022

Description

This PR solves the problem to distinguish if batch size of an env is flexible or not

Motivation and Context

The motivation is that stateful envs have different behavious than stateless envs with respect to batch_size. Where as stateful envs have a hard constraint on env.batch_size == tensordict.batch_size it's not the case of stateless envs. Therefore we add batch_locked attribute that allow or not batch flexibility in the env

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

  • New feature (non-breaking change which adds core functionality)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • I have read the CONTRIBUTION guide (required)
  • I have updated the tests accordingly (required for a bug fix or a new feature).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2022
@nicolas-dufour nicolas-dufour changed the title [Feature] Added stateful vs stateless distinction in EnvBase [Feature] Added batch_lock attribute in EnvBase Sep 8, 2022
@vmoens vmoens added the enhancement New feature or request label Sep 8, 2022
Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

Perhaps we want to check that our envs have the attribute we want somewhere in the tests?
(like in test_env, create one gym and one model-based and assess whether they both have the correct batch_locked?)
Also we should test that this can't be set once that is implemented

@nicolas-dufour
Copy link
Contributor Author

I've added tests for gym. I'll add the same test for MB_env in the MBEnv PR #333

Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

I added a couple more comments, can you address these?

@vmoens
Copy link
Collaborator

vmoens commented Sep 13, 2022

I think tests are failing because you have not merged main into your branch recently but i might be wrong

Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

I'd like to know if you consider that TransformedEnv should make _batch_locked mutable? Can we also remove the commented lines?

@nicolas-dufour
Copy link
Contributor Author

Why would we make TransformedEnv being able to change the batch_locked. At the end of the day, base_env will still have that constraint

Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

Sorry about this, still a couple of fix to be made before landing

@vmoens vmoens changed the title [Feature] Added batch_lock attribute in EnvBase [Feature] Added batch_lock attribute in EnvBase Sep 16, 2022
Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

almost there, a couple of minor things

@vmoens
Copy link
Collaborator

vmoens commented Sep 16, 2022

@nicolas-dufour have a look at my changes, if you're ok with them we can merge

@vmoens vmoens merged commit ef1bf20 into pytorch:main Sep 16, 2022
@nicolas-dufour nicolas-dufour deleted the statefull_stateless branch September 16, 2022 12:59
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. enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants