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

[TGA] Option to block full context #2928

Merged
merged 6 commits into from
Aug 4, 2020

Conversation

klshuster
Copy link
Contributor

Patch description
Beam search context ngram blocking is currently limited by the text truncation for a given model (e.g. for Blender this is only 128 tokens). I've added a flag to allow context blocking throughout the whole dialogue history.

Testing steps
Added a test

Logs

$ pytest -k TestTreeSearch
============================================================================== test session starts ==============================================================================
platform linux -- Python 3.6.9, pytest-5.3.2, py-1.8.1, pluggy-0.13.1
rootdir: /private/home/kshuster/ParlAI, inifile: pytest.ini, testpaths: tests
plugins: requests-mock-1.7.0
collected 359 items / 358 deselected / 1 selected

tests/test_tga.py .                                                                                                                                                       [100%]

=========================================================================== slowest 10 test durations ===========================================================================
4.27s call     tests/test_tga.py::TestTreeSearch::test_full_context_block

(0.00 durations hidden.  Use -vv to show these durations.)
================================================================= 1 passed, 358 deselected, 5 warnings in 9.27s =================================================================

Copy link
Contributor

@EricMichaelSmith EricMichaelSmith left a comment

Choose a reason for hiding this comment

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

Wow - so fast! Super helpful, thanks :)

ctxt = batch.text_vec[batch_idx]
if self.beam_block_full_context:
full_ctxt = batch.observations[batch_idx].get('full_text_vec', ctxt)
if not isinstance(full_ctxt, torch.Tensor):
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm - should it be if not isinstance(full_ctxt, torch.LongTensor): instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it's just a check that it's not a List; will update

@klshuster
Copy link
Contributor Author

klshuster commented Aug 4, 2020

Default is now True (cc @jaseweston); re-requesting review for changes to upgrade_opt

Copy link
Contributor

@EricMichaelSmith EricMichaelSmith left a comment

Choose a reason for hiding this comment

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

Looks reasonable, thanks!

# from generating ngrams from model's context, which is limited
# by truncation parameters. Now, we block on full dialogue history.
if 'beam_block_full_context' not in opt_from_disk:
opt_from_disk['beam_block_full_context'] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm should we perhaps print a message about this so that people know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added

@klshuster
Copy link
Contributor Author

failing test is flaky and gpu tests are still broken; going to merge anyway (everything passed before, and nothing functionally changed besides upgrade_opt, which I am pretty sure works)

@klshuster klshuster merged commit 3d3386c into facebookresearch:master Aug 4, 2020
@klshuster klshuster deleted the block_full_ctxt branch August 4, 2020 21:48
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