Skip to content

[BugFix] Parametric collectors #1303

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jun 29, 2023
Merged

[BugFix] Parametric collectors #1303

merged 14 commits into from
Jun 29, 2023

Conversation

matteobettini
Copy link
Contributor

No description provided.

@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 Jun 21, 2023
@matteobettini matteobettini mentioned this pull request Jun 21, 2023
Merged
vmoens
vmoens reviewed Jun 22, 2023
View reviewed changes
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.

Could you add a test (make sure it's a short one, ie just one worker for the mp collectors)

matteobettini
matteobettini commented Jun 22, 2023
View reviewed changes
test/test_env.py
assert ("next", *env.reward_key) in next_state.keys(True)

check_env_specs(env)
# check_env_specs(env)
Copy link
Contributor Author

@matteobettini matteobettini Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this needs #1268 fixed to be uncommented

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@matteobettini
Copy link
Contributor Author

I have added the test (if it s too long feel free to cut some parts of it).
It uses an inproved version of NestedCountinEnv which will help us test nested envs with an added dimensionality for nested keys (just like MARL)

@matteobettini matteobettini marked this pull request as draft June 22, 2023 19:04
matteobettini
matteobettini commented Jun 22, 2023
View reviewed changes
test/test_collector.py
break
ccollector.shutdown()

# assert ("data","reward") not in td.keys(True) # this can be activates once step_mdp is fixed for nested keys
Copy link
Contributor Author

@matteobettini matteobettini Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this needs #1268 fixed to be uncommented

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, afaic we can merge it and uncomment later

@matteobettini matteobettini marked this pull request as ready for review June 22, 2023 20:32
@matteobettini
Copy link
Contributor Author

I have added more general tests for nested envs and specific collector tests

matteobettini
matteobettini commented Jun 23, 2023
View reviewed changes
test/test_env.py
MockBatchedUnLockedEnv,
MockSerialEnv,
NestedRewardEnv,
# NestedCountingEnv,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needs #1268 fixed to be uncommented

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@matteobettini matteobettini force-pushed the parametric_collectors branch from fabb85f to 4958d8d Compare June 28, 2023 10:05
@vmoens vmoens added the bug Something isn't working label Jun 28, 2023
vmoens
vmoens approved these changes Jun 28, 2023
View reviewed changes
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.

LGTM thanks!

test/test_collector.py
break
ccollector.shutdown()

# assert ("data","reward") not in td.keys(True) # this can be activates once step_mdp is fixed for nested keys
Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, afaic we can merge it and uncomment later

test/test_env.py
assert ("next", *env.reward_key) in next_state.keys(True)

check_env_specs(env)
# check_env_specs(env)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

test/test_env.py
MockBatchedUnLockedEnv,
MockSerialEnv,
NestedRewardEnv,
# NestedCountingEnv,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@matteobettini matteobettini mentioned this pull request Jun 28, 2023
Merged
@vmoens
Copy link
Collaborator

vmoens commented Jun 28, 2023

Let's reduce the test time and we're good to go!

@matteobettini
Copy link
Contributor Author

Is it still too high after the commit named slimmer test?

@vmoens vmoens merged commit 36d2478 into pytorch:main Jun 29, 2023
@matteobettini matteobettini deleted the parametric_collectors branch June 29, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmoens vmoens vmoens approved these changes

Assignees

No one assigned

Labels

bug Something isn't working 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@matteobettini @vmoens @facebook-github-bot