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

Scorer model loading #860

Merged
merged 25 commits into from
Jan 18, 2022
Merged

Conversation

graemenail
Copy link
Member

@graemenail graemenail commented Apr 20, 2021

Description

When creating scorers, there are two calls to loadItems:

  1. getYamlFromModel loads the 'special' YAML from the model
  2. When the scorer is created, a EncoderDecoder method results in a call to ExpressionGraph::load

This PR addresses these by calling io::loadItems in advance and passing the Item vector when creating scorers.

  1. getYamlFromModel can now be called directly from an Item vector
  2. ScorerWrapper can be constructed from an Item vector.

Amun and Nematus do some specific preprocessing before loading the graph, this PR moves that logic into a load from Items.

Closes #831

List of changes:

  • Adds --model-mmap as a command line option (uses mio)
  • Preloads model to Item vector before creating scorers
  • try-catch when loading model YAML is replaced with if-else
  • Extended IEncoderDecoder to load from Item vector
  • Amun and Nematus models load overload for Item vector

Added dependencies: None

How to test

Used marian-conv to test mmap loading
Loading from Items passes regression tests

Checklist

  • I have tested the code manually
  • I have run regression tests
  • I have read and followed CONTRIBUTING.md
  • I have updated CHANGELOG.md

@graemenail graemenail force-pushed the scorer-model-loading branch from 1a83f84 to e6574ad Compare April 26, 2021 15:11
@graemenail graemenail marked this pull request as ready for review April 28, 2021 09:14
Copy link
Member

@snukky snukky left a comment

Choose a reason for hiding this comment

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

Looks good, only minor comments from me. Thanks! I will ask @emjotde to take a look too.

@snukky snukky requested a review from emjotde April 28, 2021 11:54
@snukky
Copy link
Member

snukky commented Apr 28, 2021

Please add an entry about the model-mmap option to CHANGELOG.

@snukky
Copy link
Member

snukky commented Jan 14, 2022

This regression test fails for me with this PR:

tests/training/restarting/test_restarting_finished.sh

It fails because logs no longer have the message saying 'Loading model' when trying to restart a training that has already finished. @graemenail, Do you think this is expected now? If so, I will just update the test.

This was tested after rebasing with the current master. That's the only regression test that fails.

@graemenail
Copy link
Member Author

This regression test fails for me with this PR:

tests/training/restarting/test_restarting_finished.sh

It fails because logs no longer have the message saying 'Loading model' when trying to restart a training that has already finished. @graemenail, Do you think this is expected now? If so, I will just update the test.

This was tested after rebasing with the current master. That's the only regression test that fails.

Thanks for looking at this @snukky. I believe this is expected, the Loading model from {} log messages contained the filename of the model loaded. In this PR, the model is loaded beforehand, and so the filename is replaced with the vector<io:Items> from that file.

That being said, I think it's a useful log message to include. I'll see where is best to reinstate it, and update this PR.

@graemenail
Copy link
Member Author

graemenail commented Jan 17, 2022

I have reinstated the log messages for Amun and Nematus, which should fix the regression.
I have also added log messages when loading model ahead of createScorer as well.

@snukky snukky merged commit b29cc07 into marian-nmt:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Loading a model currently loads the model 2Xnum_threads times
2 participants