Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

More help info for archive_surgery.py #2327

Merged
merged 21 commits into from Jan 31, 2019

Conversation

WrRan
Copy link
Contributor

@WrRan WrRan commented Jan 10, 2019

Before, typing python scripts/archive_surgery.py --help, got:

usage: archive_surgery.py [-h] --input-file INPUT_FILE --output-file
                          OUTPUT_FILE [--editor EDITOR]

Perform surgery on a model.tar.gz archive

optional arguments:
  -h, --help            show this help message and exit
  --input-file INPUT_FILE
  --output-file OUTPUT_FILE
  --editor EDITOR

Now, you will get:

usage: archive_surgery.py [-h] --input-file INPUT_FILE --output-file
                          OUTPUT_FILE [--editor EDITOR]

Perform surgery on a model.tar.gz archive

optional arguments:
  -h, --help            show this help message and exit
  --input-file INPUT_FILE
                        path to input file (default: None)
  --output-file OUTPUT_FILE
                        path to output file (default: None)
  --editor EDITOR       editor to launch, whose default value is the environment
                        vaiable `$EDITOR` (default: vim)

May help users to understand the meaning and state of the parameters.

SivilTaram and others added 16 commits January 6, 2019 15:07
* script for doing archive surgery

* simplify script
* Update the elmo command help text.

* Update the find-lr subcommand help text.
As it currently stands, the following is logged during training:

```
2019-01-06 10:46:21,832 - INFO - allennlp.common.from_params - instantiating class <class 'allennlp.model
s.language_model.LanguageModel'> from params {'bidirectional': False, 'contextualizer': {'bidirectional':
 False, 'dropout': 0.5, 'hidden_size': 200, 'input_size': 200, 'num_layers': 2, 'type': 'lstm'}, 'dropout
': 0.5, 'text_field_embedder': {'token_embedders': {'tokens': {'embedding_dim': 200, 'type': 'embedding'}
}}} and extras {'vocab': <allennlp.data.vocabulary.Vocabulary object at 0x7ff7811665f8>}
```

Note that the `Vocabulary` does not provide any useful information, since it doesn't have `__repr__` defined. This provides a fix.
…#2264)

Fixes allenai#2255

This PR replaces the `BidirectionalLM` class with a more-general `LanguageModel` that can be used in either the unidirectional/forward setting or the bidirectional setting.

It also accordingly replaces the `BidirectionalLanguageModelTokenEmbedder` with a `LanguageModelTokenEmbedder`.

Also fixes bug in the experiment_unsampled.jsonnet config that was preventing a test from actually being unsampled.

TODO:

- [x] test the unidirectional case
- [x] properly deprecate `BidirectionalLM` and `BidirectionalLanguageModelTokenEmbedder` 
- [x] check docs for accuracy
- [x] fix user-facing training configs
Merge from allenai/allennlp master
Copy link
Contributor

@joelgrus joelgrus left a comment

Choose a reason for hiding this comment

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

I find the new interplay between force and output-file and inplace very confusing. If I do inplace do I need to force or not? I can read the code, but it's not obvious from the names or the documentation.

What can we do to make this cleaner / easier to understand?

raise RuntimeError("please specify an editor or set the $EDITOR environment variable")

if os.path.exists(args.output_file):
if not args.inplace and os.path.exists(args.output_file) and not args.force:
raise ValueError("output file already exists")
Copy link
Member

Choose a reason for hiding this comment

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

"output file already exists, use --force to override"

@schmmd
Copy link
Member

schmmd commented Jan 15, 2019

@joelgrus the args seem reasonable to me. You can assume you don't need force until an error message tells you to use it (once my comment is addressed). You don't need force with inplace.

@WrRan
Copy link
Contributor Author

WrRan commented Jan 16, 2019

Or change the help string of --inplace into "overwrite the input file with the modified configuration"? Is it clearer?

@schmmd
Copy link
Member

schmmd commented Jan 16, 2019

@WrRan that strikes me as an improvement!

@WrRan
Copy link
Contributor Author

WrRan commented Jan 17, 2019

Thanks for your feedback. 😄

@schmmd
Copy link
Member

schmmd commented Jan 17, 2019

@joelgrus does this look good to you?

@WrRan
Copy link
Contributor Author

WrRan commented Jan 23, 2019

Another question, should we put this very useful scripts under allennlp/allennlp/scripts/ directory?
If we do this, we can call this script as follow:

python -m allennlp.scripts.archive_surgery --help

Personally, I think it will be more convenient.

What do you think? And thanks for your attention, again. 😃

@DeNeutoy DeNeutoy merged commit 84eb4d1 into allenai:master Jan 31, 2019
@WrRan WrRan deleted the more_help_for_archive_surgery branch March 16, 2019 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants