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

Fix terminal size at 80x24, write bzip2'ed ttyrecs #97

Merged
merged 3 commits into from
Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ commands:
command: |
sudo apt-get install software-properties-common
sudo apt-get update -y
sudo apt-get install -y libncurses5 libncurses5-dev flex bison cmake
sudo apt-get install -y libncurses5 libncurses5-dev flex bison cmake libbz2-dev

pip-install-nle:
description: Installs NLE and its dependencies
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ target_link_directories(nethack PUBLIC /usr/local/lib)

# On Ubuntu 20.04, libncurses6 seems to cause a SEGFAULT on tgetent?
find_library(NCURSES_LIB NAMES libncurses.so.5 libncurses ncurses)
target_link_libraries(nethack PUBLIC m fcontext ${NCURSES_LIB})
target_link_libraries(nethack PUBLIC m fcontext ${NCURSES_LIB} bz2)

# dlopen wrapper library
add_library(nethackdl STATIC "sys/unix/nledl.c")
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ can be installed by doing:
```bash
# Python and most build deps
$ sudo apt-get install -y build-essential autoconf libtool pkg-config \
python3-dev python3-pip python3-numpy git libncurses5-dev flex bison
python3-dev python3-pip python3-numpy git libncurses5-dev flex bison libbz2-dev

# recent cmake version
$ wget -O - https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | sudo apt-key add -
Expand Down
6 changes: 6 additions & 0 deletions include/nle.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#ifndef NLE_H
#define NLE_H

#define NLE_BZ2_TTYRECS

#include <stdio.h>

#include <fcontext/fcontext.h>
Expand All @@ -21,6 +23,10 @@ typedef struct nle_globals {
char *outbuf_write_ptr;
char *outbuf_write_end;

#ifdef NLE_BZ2_TTYRECS
void *ttyrec_bz2;
#endif

Comment on lines +26 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifdef NLE_BZ2_TTYRECS
void *ttyrec_bz2;
#endif
#ifdef NLE_BZ2_TTYRECS
void *ttyrec_bz2;
#else
FILE *ttyrec;
#endif

I'm curious: why aren't we doing ^^^ ? We never write to the file if the macro is set. Do we not just run the risk of confusion by keeping this pointer around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Who exactly is responsible for this FILE is a bit of a tricky question. For now, I'd like to keep it around for debugging and to allow flushing (although we don't do that currently).

boolean done;
nle_obs *observation;
} nle_ctx_t;
Expand Down
2 changes: 1 addition & 1 deletion include/unixconf.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@
#define TIMED_DELAY
#endif

/* #define AVOID_WIN_IOCTL */ /* ensure USE_WIN_IOCTL remains undefined */
#define AVOID_WIN_IOCTL /* ensure USE_WIN_IOCTL remains undefined */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment could probably be added to, to help future clarity.

Suggested change
#define AVOID_WIN_IOCTL /* ensure USE_WIN_IOCTL remains undefined */
/*
* ensure USE_WIN_IOCTL remains undefined
* USE_WIN_IOCTL can set LI and CO global variables for terminal size, which we fix in nle.c
*/
#define AVOID_WIN_IOCTL

Copy link
Contributor

Choose a reason for hiding this comment

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

formatting of the comment can obvs change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see where you're coming from, but I'd like to stick with the policy of not unduly changing the original NetHack source files, and this is one of them.


/*
* If you define MAIL, then the player will be notified of new mail
Expand Down
2 changes: 1 addition & 1 deletion nle/env/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def __init__(

if self.savedir:
self._ttyrec_pattern = os.path.join(
self.savedir, "nle.%i.%%i.ttyrec" % os.getpid()
self.savedir, "nle.%i.%%i.ttyrec.bz2" % os.getpid()
Copy link
Contributor

Choose a reason for hiding this comment

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

pid is always going to be unique within the folder? Are we saving all ttyrecs to there or only the last N?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we are either saving all ttyrecs or none.

The pid is going to be the process id of the user's process which won't be reassigned until that process stops.

)
ttyrec = self._ttyrec_pattern % 0
else:
Expand Down
2 changes: 1 addition & 1 deletion nle/nethack/nethack.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def __init__(
self,
observation_keys=OBSERVATION_DESC.keys(),
playername="Agent-mon-hum-neu-mal",
ttyrec="nle.ttyrec",
ttyrec="nle.ttyrec.bz2",
options=None,
copy=False,
wizard=False,
Expand Down
2 changes: 1 addition & 1 deletion nle/scripts/play.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def play(env, mode, ngames, max_steps, seeds, savedir, no_render, debug):
if is_raw_env:
if savedir is not None:
os.makedirs(savedir, exist_ok=True)
ttyrec = os.path.join(savedir, "nle.ttyrec")
ttyrec = os.path.join(savedir, "nle.ttyrec.bz2")
else:
ttyrec = "/dev/null"
env = nethack.Nethack(ttyrec=ttyrec)
Expand Down
2 changes: 1 addition & 1 deletion nle/tests/test_envs.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ def test_rollout(self, env_name, rollout_len):
env.close()

assert os.path.exists(
os.path.join(savedir, "nle.%i.0.ttyrec" % os.getpid())
os.path.join(savedir, "nle.%i.0.ttyrec.bz2" % os.getpid())
)

def test_rollout_no_archive(self, env_name, rollout_len):
Expand Down
65 changes: 47 additions & 18 deletions src/nle.c
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@

#include <assert.h>
#include <sys/time.h>

#include <string.h>
#include <sys/time.h>

#define NEED_VARARGS
#include "hack.h"
Expand All @@ -11,6 +10,10 @@

#include "nle.h"

#ifdef NLE_BZ2_TTYRECS
#include <bzlib.h>
#endif

#define STACK_SIZE (1 << 15) // 32KiB

#ifndef __has_feature
Expand All @@ -31,6 +34,12 @@ init_nle(FILE *ttyrec)
assert(ttyrec != NULL);
nle->ttyrec = ttyrec;
heiner marked this conversation as resolved.
Show resolved Hide resolved

#ifdef NLE_BZ2_TTYRECS
int bzerror;
nle->ttyrec_bz2 = BZ2_bzWriteOpen(&bzerror, ttyrec, 9, 0, 0);
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 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert(bzerror == BZ_OK);
#endif

nle->outbuf_write_ptr = nle->outbuf;
nle->outbuf_write_end = nle->outbuf + sizeof(nle->outbuf);

Expand Down Expand Up @@ -59,6 +68,20 @@ mainloop(fcontext_transfer_t ctx_transfer)
unixmain(1, argv);
}

boolean
write_data(void *buf, int length)
{
nle_ctx_t *nle = current_nle_ctx;
#ifdef NLE_BZ2_TTYRECS
int bzerror;
BZ2_bzWrite(&bzerror, nle->ttyrec_bz2, buf, length);
assert(bzerror == BZ_OK);
#else
assert(fwrite(buf, 1, length, nle->ttyrec) == 0);
#endif
return TRUE;
}

boolean
write_header(int length, unsigned char channel)
{
Expand All @@ -70,18 +93,9 @@ write_header(int length, unsigned char channel)
buffer[1] = tv.tv_usec;
buffer[2] = length;

nle_ctx_t *nle = current_nle_ctx;

/* Assumes little endianness */
if (fwrite(buffer, sizeof(int), 3, nle->ttyrec) == 0) {
assert(FALSE);
return FALSE;
}

if (fputc((int) channel, nle->ttyrec) != (int) channel) {
assert(FALSE);
return FALSE;
}
write_data(buffer, 3 * sizeof(int));
write_data(&channel, 1);

return TRUE;
}
Expand All @@ -102,12 +116,17 @@ nle_fflush(FILE *stream)
ssize_t length = nle->outbuf_write_ptr - nle->outbuf;
if (length == 0)
return 0;
/* TODO(heiner): Given that we do our own buffering, consider
* using file descriptors instead of the ttyrec FILE*. */

write_header(length, 0);
fwrite(nle->outbuf, 1, length, nle->ttyrec);
write_data(nle->outbuf, length);

nle->outbuf_write_ptr = nle->outbuf;

#ifdef NLE_BZ2_TTYRECS
return 0;
#else
return fflush(nle->ttyrec);
#endif
}

/*
Expand Down Expand Up @@ -219,6 +238,10 @@ init_random(int FDECL((*fn), (int) ))
nle_ctx_t *
nle_start(nle_obs *obs, FILE *ttyrec, nle_seeds_init_t *seed_init)
{
/* Set CO and LI to control ttyrec output size. */
CO = 80;
LI = 24;

nle_ctx_t *nle = init_nle(ttyrec);
nle->observation = obs;
nle_seeds_init = seed_init;
Expand All @@ -244,7 +267,7 @@ nle_step(nle_ctx_t *nle, nle_obs *obs)
current_nle_ctx = nle;
nle->observation = obs;
write_header(1, 1);
fputc(obs->action, nle->ttyrec);
write_data(&obs->action, 1);
fcontext_transfer_t t = jump_fcontext(nle->generatorcontext, obs);
nle->generatorcontext = t.ctx;
nle->done = (t.data == NULL);
Expand All @@ -256,7 +279,6 @@ nle_step(nle_ctx_t *nle, nle_obs *obs)
void
nle_end(nle_ctx_t *nle)
{
nle_fflush(stdout);
if (!nle->done) {
/* Reset without closing nethack. Need free memory, etc.
* this is what nh_terminate in end.c does. I hope it's enough. */
Expand All @@ -265,6 +287,13 @@ nle_end(nle_ctx_t *nle)
dlb_cleanup();
}
}
nle_fflush(stdout);

#ifdef NLE_BZ2_TTYRECS
int bzerror;
BZ2_bzWriteClose(&bzerror, nle->ttyrec_bz2, 0, NULL, NULL);
assert(bzerror == BZ_OK);
#endif

destroy_fcontext_stack(&nle->stack);
free(nle);
Expand Down