Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Fix _underscore access, rename Nethack variable as nethack. #303

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

heiner
Copy link
Contributor

@heiner heiner commented Jan 25, 2022

Fixes #273.

One reason this matters more than one would assume is that gym envs wrappers tend to forward non-underscore access but will typically fail with ._underscore access. This happened e.g. for the 0.21 release which auto-wrapped every env.

Also a bit more cleanup.

@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 Jan 25, 2022
Copy link
Contributor

@cdmatters cdmatters left a comment

Choose a reason for hiding this comment

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

No more env.env! 🎉

@@ -312,15 +310,15 @@ def __init__(
else:
ttyrec = None

self.env = nethack.Nethack(
self.nethack = nethack.Nethack(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a much better renaming, though im sure will annoy downstream users.

Copy link
Contributor

Choose a reason for hiding this comment

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

so funny how easy it is to ignore "call it what it is!"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true re: downstream users. Hence I at least included @samvelyan here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could include an alias.

Then again, this is easy to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for flagging this @heiner. I'll incorporate this into MiniHack after NLE's next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, if it's easier we can add an alias.

Copy link

Choose a reason for hiding this comment

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

Hi team, do you know when there will be the next release of MiniHack? I'm trying to use recent nle because of issue fixed by #299

@heiner heiner merged commit f4c0e55 into main Jan 26, 2022
@heiner heiner deleted the heiner/fix_underscores branch January 26, 2022 12:25
@samvelyan
Copy link
Contributor

Seems like this PR brakes the play.py script which uses env._actions. This is a small fix.

@heiner
Copy link
Contributor Author

heiner commented Jan 26, 2022

Seems like this PR brakes the play.py script which uses env._actions. This is a small fix.

Good catch, thanks! Fixed in #308

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

Don't rely on access to _underscored variables
5 participants