Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[World logging] Fix world logging for dynamic batching #3867

Merged
merged 5 commits into from
Aug 8, 2021

Conversation

emilydinan
Copy link
Contributor

Patch description
Episodes were not properly being flushed during world logging since world.acts always returns [None, None] -- acts are only tracked at the DynamicBatchWorld level. Added a manual check in world logging for a parley to see if the episode is done.

Testing steps
Added a test that broke before the change and passes after.

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

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

Thx seems reasonable

jxmsML
jxmsML previously requested changes Jul 27, 2021
dict(
model_file='zoo:unittest/transformer_generator2/model',
task='integration_tests:RepeatTeacher:2000',
world_logs=save_report,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: world_logs = save_report + '.jsonl'

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about that?

Copy link
Contributor

@jxmsML jxmsML Jul 27, 2021

Choose a reason for hiding this comment

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

https://github.com/facebookresearch/ParlAI/blob/master/tests/test_eval_model.py#L214-L215
is it here that world_logs and report_filename share the same path?

tests/test_dynamicbatching.py Outdated Show resolved Hide resolved
tests/test_dynamicbatching.py Outdated Show resolved Hide resolved
@stephenroller stephenroller merged commit b414f39 into master Aug 8, 2021
@stephenroller stephenroller deleted the dynbatchfix branch August 8, 2021 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants