Binary shortlist #856
Merged
Binary shortlist #856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nto binary_shortlist
Make hash arch-independent
Enable mpi wrapper to use size larger than MAX_INT.
…lation/Marian/marian-dev into hihoan/lsh7
…lation/Marian/marian-dev into hihoan/lsh7
This PR unifies the shortlist and LSH interface achieving significant speed-up for the LSH.
…uicksand This PR turns the LSH index and search into a set of operators that live in the expression graph. This makes creation etc. thread-safe (one index per graph) and allows to later implement GPU versions. This allows to mmap the LSH as a Marian parameter since now we only need to turn the index into something that can be saved to disk using the existing tensors. This happens in marian_conv or the equivalent interface function in the Quicksand interface.
|
@hieuhoang can you look at the merge conflict in shortlist.cpp? I will look into marian_conv |
|
regression past, the usual tests failed:
|
hieuhoang
approved these changes
Jul 10, 2021
|
Hm, looks like Github didn't realize that we updated master too and now shows the merge as changes. |
emjotde
approved these changes
Jul 10, 2021
src/command/marian_conv.cpp
Outdated
| cli->add<std::vector<std::string>>("--vocabs,-V", "Vocabulary file, required for ONNX export"); | ||
| cli->add<std::vector<std::string>>("--shortlist,-s", "Shortlist conversion: filePath firstNum bestNum threshold"); | ||
| cli->add<std::string>("--dump,-d", "Binary shortlist dump path","lex.bin"); |
There was a problem hiding this comment.
Let's rename this to dump-shortlist, dump is too generic.
src/command/marian_conv.cpp
Outdated
| Ptr<const data::ShortlistGenerator> binaryShortlistGenerator | ||
| = New<data::BinaryShortlistGenerator>(options, srcVocab, trgVocab, 0, 1, vocabPaths[0] == vocabPaths[1]); | ||
| binaryShortlistGenerator->dump(dumpPath); | ||
| LOG(info, "Finished"); |
There was a problem hiding this comment.
Similarly as above, mention that it's the dumping of the shortlist that finished, otherwise a bit too generic.
| @@ -1,5 +1,8 @@ | |||
| #include "data/shortlist.h" | |||
| #include "microsoft/shortlist/utils/ParameterTree.h" | |||
| #include "marian.h" | |||
| #include "layers/lsh.h" | |||
| #include <queue> | |||
There was a problem hiding this comment.
Nitpick: we usually add an empty line between our and system headers.
|
@qianqianzhu after the option is renamed this is good to merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a new binary shortlist generator which allows the decoder to load a lexical shortlist from a binary blob.
The new binary shortlist can greatly improve the lexical shortlist loading time (x4309 speed-up).
This feature has been integrated into the bergamot branch.
List of changes:
BinaryShortlistGeneratorClass to load lexical shortlist from a binary blob.To use a binary shortlist, you should provide the file path and whether the file needs checking (optional; true by default).
For example:
marian-convwith--shortlist,-soptionShortlist conversion usage example:
hash.hfor checksum calculationAdded dependencies: none
How to test
Manually tested new BinaryShortlistGenerator with LexicalShortlistGenerator. The results are the same. Also passed unit tests.
Regression tests are passed with gcc 8.4.0 and cuda 10.1.
Checklist