-
Notifications
You must be signed in to change notification settings - Fork 9
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
Quick fix to allow using nethack config files #15
base: main
Are you sure you want to change the base?
Conversation
Haven't gone into this much yet, but does line 2016 in files.c stop NLE from processing that NetHack config file? |
It probably does. We tried to limit the non-package state that influences nle at the time (people seem to find even time/date problematic...). It's sad there's no env variable mechanism to change these settings :/. Ideally we'd control it well enough to not need parsing at each restart (RL environments are different from games in that they are restarted far, far more often, and the code here wasn't written in a way to be very efficient at restarts.) |
Oh yes, it does look like it's attempting to do so. However I have been using I also totally agree that it would be great to have variables to provide proper control, but I imagine this would require a much deeper overhaul. |
Maybe update the main README to have a citation guideline to include any .nethackrc settings so others can follow the work? |
@@ -206,7 +206,13 @@ def __init__( | |||
|
|||
if options is None: | |||
options = NETHACKOPTIONS | |||
self.options = list(options) + ["name:" + playername] | |||
|
|||
self.options = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
self.options = list(options)
if options[0][0] != "@":
self.options.append(f"name:{playername}")
and perhaps also a comment what this does.
And perhaps a test to show this works? I'm still a bit unclear if this is a good idea as it pulls in yet another external source of state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to the file as well to the README file.
I think having the config file could potentially help manage NetHack experiments, where a configuration file would be associated with a certain experiment.
Since the config file also allows changing the visual aspect of NetHack it could also allow for testing transfer and generalization across that modality.
But yeah, it would be a advanced feature of the NLE, most people would probably not touch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
This is a quick fix to allow users to pass their Nethack options as a a configuration file (please see https://nethackwiki.com/wiki/Options) which has to be referred through
options = ("@.nethackrc")
where.nethackrc
is the config file. The current code breaks as it only expects a tuple of options.The difference with the current solution (say using this tuple of options https://github.com/heiner/nle/blob/main/nle/nethack/nethack.py#L53) is that a config file allows for greater customization of options, for example through specifications where not only
OPTIONs
is handled, but alsoAUTOPICKUP_EXCEPTION
. here is an example below.Most Nethack players use such configurations, for example this one.