Fixes to IntentSlots init logic making all TLT scripts operational
State: closed | Pull request: yes

This is still work in progress - but at least:

- [x] CI passes
- [x] whole TLT workflow (train, finetune, evaluate, inference, export, infer ONNX scripts) work
- [x] setting datadir also "reconfigures" the model (e.g. slots/intents change => heads change too)
- [x] **!! change in the way ModelPT is handing artifacts!!** => saving original path despite whether artifact was restored from that path or from "./"

Still, the whole logic of the model is just horrible. The major problem I see here is improper `separation of concerns`:
1) there is improper separation of parameters - plenty of parameters are shared between model and dataset/dataloader related sections => each `setup_*_ds` should receive all the params required to create ds&dl

2) there is DataDescriptor that does magical operations on slots and values from both training and validation splits => those values should be gathered together during the data preparation phase, and `setup_train_ds` should load them, store in config and other `setup_*_ds` should simply use them

3) DataDescriptor is needed to be created before calling parent class constructor (which calls `setup_*_ds`) and it requires slots and values from both splits to be loaded first. This creates an chicken and egg problem - get rid of DataDescriptor and follow logic from 2) to get rid of that cycle.

4) model reconfiguration should change only last layers of classifier - now it is (re)instantiating the whole classifier object, which IMO is not the best design...


@titu1994 The fix comes from me doing the cycles (save - restore - save), which is not a _typical_ NeMo use-case, but is a typical TLT workflow. Thanks for the comment though.


Save restore save is the general cycle of NeMo too - save when we train a model, convert to .Nemo and upload, use from_pretrained(), change vocabulary, fine-tune, save_to() again. 

It's not just a TLT workflow, so please test this on an empty environment which doesn't have the files in exactly the locations that save was called. 

So save on one machine, move to another, restore from in that machine and save as well. If no issues are there during restore on a second machine, all's good. 

> So save on one machine, move to another, restore from in that machine and save as well. If no issues are there during restore on a second machine, all's good.

Has _anyone_ tested that use-case?

Just perform a "mental experiment" and go through (restore - save) cycle in your head: any idea where `save_to()` method will take the "restored" artifact that you will need to add to the archive during saving? 

> Save restore save is the general cycle of NeMo too - save when we train a model, convert to .Nemo and upload, use from_pretrained(), change vocabulary, fine-tune, save_to() again.

This is a slightly different case as you are setting new artifact, so the old one, that is in archive, won't be needed anymore.

But let me repeat the same question: has _anyone_ tested this use-case?

Please perform 2nd "mental experiment": If the model reconfiguration is not happening in the model constructor (e.g. ASR Models have a separate method for this) the current `register_artifacts` code will **append** the second artifact, not overwrite the old one... Is that the desired behavior?

Summarizing, I like the idea of artifacts, but I also see plenty of problems with the way they currently work. And I have some ideas how to solve those problems, plus would like to add some additional functionality to them  and hope we will discuss all that some time soon.

But fixing artifacts in general is IMO out of scope of this PR - the goal here was to incorporate changes enabling to have the whole TLT workflow, with artifacts passed along different scripts, operational. This tiny change, along with some work on EFF and TLT sides, made it work.

@okuchaiev makes sense? can we merge?

> So save on one machine, move to another, restore from in that machine and save as well. If no issues are there during restore on a second machine, all's good.

ASR Notebooks 1, 2, 4 and I believe 6 does this. ASR 1 (finetuning section) is basically the above workflow - where second environment is Colab and 1st is our local train environment. 

> Just perform a "mental experiment" and go through (restore - save) cycle in your head: any idea where save_to() method will take the "restored" artifact that you will need to add to the archive during saving?

1) Load model using hydra, train, finish, call save_to() - saves the local path of the tokenizer into config + actual tokenizer vocab into the archive.

2) Restore model using restore_from inside colab (or equivalent from_pretrained) - the code reperforms register_artifact() - this time the actual value doesnt exist in the old local path, so it instead searches and finds the new path (inside the tarfile). Returns that path *** This new path is required by all tokenizers and is what is being returned from this function - prior to this PR change.

3) The model gets the new (tarpath filepath) and submits it to create a tokenizer from the vocabulary present in the local path.

The entire workflow above is contingent on the fact that the returned path is the **new** (tarpath filepath) of the tokenizer vocabulary. So your change breaks all of ASR BPE.

The tests dont capture this - because as I said, the test environment still has the original tokenizer vocab after save and durin restore. So it never needs to do the second half of step (2) - the original filepath is accurate and goes through without finding the path from the tarfile.

> But let me repeat the same question: has anyone tested this use-case?

Yes - all of ASR BPE models are using this.

> Please perform 2nd "mental experiment": If the model reconfiguration is not happening in the model constructor (e.g. ASR Models have a separate method for this) the current register_artifacts code will append the second artifact, not overwrite the old one... Is that the desired behavior?

That seperate method is only called by the constructor or change_vocabulary(). They do not append any data - nor is there overwriting. The save_to path is different from the restore_from path. However, even if save_to path is same as restore_from path, the tarfile created will simply overwrite the old tarfile, so still there is no appending, just overwriting.


Som, I am not saying that the model (restore->save) is not working. And maybe I am missing something, but as far as I know the notebooks you mentioned are not doing any of the discussed use-cases. 

Let's take that off-line, this is super inefficient for discussions requiring _alignment on problem definition_. 

@tkornuta-nvidia  before a proper review, quick question - will this make "save-restore-save" cycle work for "save (machine A)-restore(Machine B)-save" ?