Using a serialisation directory as a model archive doesn't work with files_to_archive.json

[DeNeutoy]

Stack trace:

Traceback (most recent call last):
  File "/home/vidurj/miniconda3/envs/allennlp/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/home/vidurj/miniconda3/envs/allennlp/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/vidurj/allennlp/allennlp/run.py", line 18, in <module>
    main(prog="python -m allennlp.run")
  File "/home/vidurj/allennlp/allennlp/commands/__init__.py", line 63, in main
    args.func(args)
  File "/home/vidurj/allennlp/allennlp/commands/predict.py", line 158, in _predict
    predictor = _get_predictor(args)
  File "/home/vidurj/allennlp/allennlp/commands/predict.py", line 104, in _get_predictor
    overrides=args.overrides)
  File "/home/vidurj/allennlp/allennlp/models/archival.py", line 149, in load_archive
    cuda_device=cuda_device)
  File "/home/vidurj/allennlp/allennlp/models/model.py", line 233, in load
    model = Model.from_params(vocab, model_params)
  File "/home/vidurj/allennlp/allennlp/models/model.py", line 184, in from_params
    model = cls.by_name(choice).from_params(vocab, params)
  File "/home/vidurj/allennlp/allennlp/models/encoder_decoders/simple_seq2seq.py", line 373, in from_params
    source_embedder = TextFieldEmbedder.from_params(vocab, source_embedder_params)
  File "/home/vidurj/allennlp/allennlp/modules/text_field_embedders/text_field_embedder.py", line 56, in from_params
    return cls.by_name(choice).from_params(vocab, params)
  File "/home/vidurj/allennlp/allennlp/modules/text_field_embedders/basic_text_field_embedder.py", line 62, in from_params
    token_embedders[key] = TokenEmbedder.from_params(vocab, embedder_params)
  File "/home/vidurj/allennlp/allennlp/modules/token_embedders/token_embedder.py", line 32, in from_params
    return cls.by_name(choice).from_params(vocab, params)
  File "/home/vidurj/allennlp/allennlp/modules/token_embedders/elmo_token_embedder.py", line 67, in from_params
    params.add_file_to_archive('options_file')
  File "/home/vidurj/allennlp/allennlp/common/params.py", line 81, in add_file_to_archive
    self.files_to_archive[f"{self.history}{name}"] = cached_path(self.get(name))
  File "/home/vidurj/allennlp/allennlp/common/file_utils.py", line 76, in cached_path
    raise FileNotFoundError("file {} not found".format(url_or_filename))
FileNotFoundError: file seq2seq-model/fta/model.source_embedder.source_elmo.options_file not found

[DeNeutoy]

This is happening because this line: https://github.com/allenai/allennlp/blob/master/allennlp/models/archival.py#L129
is not agnostic to whether or not load_archive is being called on a serialisation directory (which won't have the fta/... path) or a model.tar.gz archive, which will. When the paths get replaced in the config, they don't exist, so when the model gets built and files_to_archive gets called from within the model code, the path in the config the model is being built from doesn't exist, which throws the above error.

[schmmd]

@DeNeutoy can you provide a repro that yield your stacktrace above?

[sai-prasanna]

@DeNeutoy I ran into this issue in datasetreader creation from archive. When using add_file_to_archive in from_params, if the model is deserialized from archive, we delete the tmp folder. In this line

allennlp/allennlp/models/archival.py

Line 157 in 64f253f

shutil.rmtree(tempdir)

.

If datasetreader has some dependency that requires one of the files from files_to_archive , it doesn't exist as we have deleted them after creating the model.

Simple fix would be to not remove the tmp folder or do it after everything is over (which would be arguably difficult).

[sai-prasanna]

@schmmd Could I submit a (temporary) fix by stopping tmp directory removal ?

[schmmd]

Sorry @sai-prasanna but I have not been following this issue closely. @DeNeutoy (you're presently assigned) what do you think of @sai-prasanna 's proposal?

It seems a bit dangerous to skip deleting the tmp directory, but clearly this issue has bitten us a bit over the past year!

[piercefreeman]

Running into the same issue as @sai-prasanna, also with re-serializing files in a dataset reader that were previously saved with files_to_archive. Is there any workaround right now?

@DeNeutoy Any thoughts on a more permanent fix?

[joelgrus]

wasn't this fixed by #2184, or is this a different bug?

[sai-prasanna]

@joelgrus - For my case its fixed by atexit hook in #2184

edit: Guess it was a different issue, where loading from model.tar.gz fails when non Model classes have used files_to_archive.

[DeNeutoy]

No, this is not fixed by that. To reproduce this, you need to train a model which utilises files_to_archive such as anything with elmo and then try and load it from the directory, not the model.tar.gz file. The problem occurs because we replace paths in the saved config.json to be references to files inside the model archive, but these don't exist if you load from a directory. I don't have a good idea of how to fix this and the code for files_to_archive quite confusing.

[joelgrus]

I see. What is the desired behavior here? If you're working from a serialization dir, then necessarily you don't have those files archived?

So one simple solution would be that if you're passed in a directory, we simply don't do that replacement. But is that the behavior you want?

[DeNeutoy]

Yeah, i'm not sure, which is partly why I haven't fixed it. I think that would work? It doesn't support the case that you move the folder around, but in that case you should probably be using a model archive anyway.