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

Glulx game logger #19

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions textworld/envs/glulx/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert unneeded change.

4 changes: 4 additions & 0 deletions textworld/envs/glulx/git_glulx_ml.py
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,10 @@ def compute_intermediate_reward(self) -> None:
def __del__(self) -> None:
self.close()

@property
def gamefile(self) -> str:
return self._gamefile

@property
def game_running(self) -> bool:
""" Determines if the game is still running. """
Expand Down
1 change: 1 addition & 0 deletions textworld/envs/wrappers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@

from textworld.envs.wrappers.viewer import HtmlViewer
from textworld.envs.wrappers.recorder import Recorder
from textworld.envs.wrappers.glulx_logger import GlulxLogger
126 changes: 126 additions & 0 deletions textworld/envs/wrappers/glulx_logger.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT license.

from typing import Tuple, List, Optional, Iterable, Union, Sized, Any, Mapping

from textworld.core import Environment, GameState, Wrapper
from textworld.envs.glulx.git_glulx_ml import GitGlulxMLEnvironment, GlulxGameState


class GlulxLogger(Wrapper):
def __init__(self, env: GitGlulxMLEnvironment) -> None:
"""
Wrap around a TextWorld GitGlulxML environment to provide logging capabilities.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap -> wrapper?


Parameters
----------
:param env:
The GitGlulxML environment to wrap.
"""
super().__init__(env)
self.activate_state_tracking()

self.serialized_game = env.game.serialize()
self._gamefile = env.gamefile


def step(self, command: str) -> Tuple[GlulxGameState, float, bool]:
"""
Take a step in the environment, save needed information.
:param command:
input string for taking an action
:return:
GlulxGameState, score and done.
"""
self._logs.append(self._current)
self._current = {'optional': []}

self._current['command'] = command

game_state, score, done = super().step(command)
self._current['feedback'] = game_state.feedback
self._current['score'] = score
self._current['done'] = done
self._current['action'] = game_state.action.serialize()
self._current['state'] = game_state.state.serialize()

return game_state, score, done

def reset(self) -> GameState:
"""
Reset the environment.
Also clears logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we say we are keeping multiple trajectories? Calling reset should append to a list of logs.

"""
self._logs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Member variables should be first declared in the __init__. That way, one can easily see what are the member variables at first glance. It also helps minimizing side-effects to some extent (e.g. logger.logs would fail before a call to env.reset() is made!).

game_state = super().reset()
self._current = {'optional': []}
self._current['done'] = False
self._current['state'] = game_state.state.serialize()

return game_state

def add_commands(self, commands: List[str], scores: Optional[Union[Iterable[float], Sized]]=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the Union? Isn't Iterable[float] enough?

"""
Add custom commands to the logger. Optionally add scores for each command.
:param commands:
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using Google docstring standard now (https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html). See, core.py for examples on how to format them.

A list of commands.
:param scores:
scores for each command. Must be same size as commands if provided.
:return:
"""
command_mapping = commands
if scores is not None:
assert len(scores) == len(commands)
command_mapping = {a: p for a, p in zip(commands, scores)}

self._current['command_distribution'] = command_mapping
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't know if the scores are normalized, so I would avoid using the term distribution. Also, if scores is None, then it's not a mapping. We could simply store commands and scores separately and it would be up to the viewer/user to deal with it accordingly.


def add(self, info: Any) -> None:
"""
Add any additional information you want to log.
:param info:
Additional information to log for the current game state.
"""
self._current['optional'].append(info)

@property
def current(self) -> Mapping:
return self._current

@property
def logs(self) -> List[Mapping]:
"""
Get all logs
:return: List of all logs
"""
logs = self._logs[:]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this line? Make a shallow copy? If so, why do you append to it before returning it?

logs.append(self._current)
return logs

@property
def gamefile(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this method. If I remember correctly, Wrappers instances automatically call method of the unwrapped environment if not defined/overwritten in the wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry what do you mean exactly? I don't see this in the Wrapper class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I was thinking of some unmerged code.

return self._gamefile

def __getitem__(self, index: int) -> Mapping:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need this anymore.

"""
Get a certain log at a given index.
:param index:
index of log to get.
:return:
log at index.
"""
assert index <= len(self._logs)

if index < len(self._logs) - 1:
return self._logs[index]
return self._current
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is _current not always the last element of _logs?


def __str__(self) -> str:
return str(self.logs)

def serialize(self) -> List[Mapping]:
"""
Get serialized mappings of logs.
:return: List of serialized mappings.
"""
return self.logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at the end of the file (PEP8).

51 changes: 51 additions & 0 deletions textworld/envs/wrappers/tests/test_glulx_logger.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT license.

import textworld
import numpy as np

from textworld.envs.wrappers import GlulxLogger
from textworld.utils import make_temp_directory
from textworld.generator import compile_game
from textworld import g_rng


def test_glulx_logger():
num_nodes = 3
num_items = 10
g_rng.set_seed(1234)
grammar_flags = {"theme": "house", "include_adj": True}
game = textworld.generator.make_game(world_size=num_nodes, nb_objects=num_items, quest_length=3, grammar_flags=grammar_flags)

game_name = "test_glulx_logger"
with make_temp_directory(prefix=game_name) as tmpdir:
game_file = compile_game(game, game_name, games_folder=tmpdir)

env = textworld.start(game_file)
env = GlulxLogger(env)
env.activate_state_tracking()
game_state = env.reset()

# test reset
assert 'state' in env.current

# test step
options = game_state.admissible_commands
game_state, score, done = env.step(options[0])
assert len(env.logs) > 1
assert 'action' in env.current
assert 'state' in env.current
assert 'feedback' in env.current

# test add_commands
option_scores = np.array([0.1] * len(options))
env.add_commands(options, option_scores)
assert len(env.current['command_distribution'].values()) == len(options)

# test add
additional_info = {'scores': option_scores}
env.add(additional_info)
assert len(env.current['optional']) > 0