Default arguments of clm example are confusing

[BramVanroy]

I was having a look at the run_clm.py script and which new arguments are available to push to the hub.

python transformers\examples\pytorch\language-modeling\run_clm.py -h

I see the following options (note the True defaults for all):

  --no_keep_linebreaks  Whether to keep line breaks when using TXT files or not. (default: True)
  --keep_linebreaks [KEEP_LINEBREAKS]
                        Whether to keep line breaks when using TXT files or not. (default: True)

  --no_dataloader_pin_memory
                        Whether or not to pin memory for DataLoader. (default: True)
  --dataloader_pin_memory [DATALOADER_PIN_MEMORY]
                        Whether or not to pin memory for DataLoader. (default: True)

  --no_skip_memory_metrics
                        Whether or not to skip adding of memory profiler reports to metrics. (default: True)
  --skip_memory_metrics [SKIP_MEMORY_METRICS]
                        Whether or not to skip adding of memory profiler reports to metrics. (default: True)

From this, I cannot figure out what the default behaviour is or what I should change to become the expected behavior. I do not know what the use case is for this but it seems much better to only keep one of each option. If one the two for each option is deprecated, then that could be added in the description too.

I'm on current master (4.12 dev).

Who can help

@sgugger, @patil-suraj

[sgugger]

Unfortunately, since the two arguments are accepted, there is no way for us to automate a better documentation of them from the HfArgumentParser (if you have ideas, by all means!) so you should rely on the documentation of TrainingArguments.

[BramVanroy]

I went looking for the no_* arguments. It seems that they are dynamically generated:

transformers/src/transformers/hf_argparser.py

Lines 112 to 113 in 3a8de58

	if field.default is True:
	parser.add_argument(f"--no_{field.name}", action="store_false", dest=field.name, **kwargs)

But I do not quite understand the use case for this. If the documentation only shows the version without no_, then why do they exist? Having two arguments for a boolean argument seems overkill.

That being said, I am sure there are reasons for that. My suggestion to make this more usable would be to negate the default value for the no_ field. This doe snot change the default behaviour as far as I tested and makes it clear to the user what the default behavior is.

import argparse

cparser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter)

cparser.add_argument("--dataloader_pin_memory", default=True, action="store_true", help="Enable memory pinning for DataLoader")
cparser.add_argument("--no_dataloader_pin_memory", default=False, action="store_false", dest="dataloader_pin_memory", help="Disable memory pinning for DataLoader")

cargs = cparser.parse_args()

print(vars(cargs))

Help will look like this (with False on the no_ option):

optional arguments:
  -h, --help            show this help message and exit
  --dataloader_pin_memory
                        Enable memory pinning for DataLoader (default: True)
  --no_dataloader_pin_memory
                        Disable memory pinning for DataLoader (default: False)

Behaviour as before:

• default: {'dataloader_pin_memory': True}
• --dataloader_pin_memory: {'dataloader_pin_memory': True}
• --no_dataloader_pin_memory: {'dataloader_pin_memory': False}

The "whether or not" in the original help description may also be confusing. Because you generate the second field dynamically, you could go so far as to be consistent with your description and simply do field_help.replace("Enable", "Disable).

[sgugger]

Like I said, the no- are automagically generated by the HfArgumentParser. We can't remove them without creating a breakign change. At the same time there is no point in adding the no- argument to the TrainingArguments class (or other dataclasses) which can also be used as is in a notebook.

[BramVanroy]

I think you misunderstood my reply. I am suggesting to change this default True:

transformers/src/transformers/hf_argparser.py

Lines 112 to 113 in 3a8de58

	if field.default is True:
	parser.add_argument(f"--no_{field.name}", action="store_false", dest=field.name, **kwargs)

into False

if field.default is True:
    parser.add_argument(f"--no_{field.name}", default=False, action="store_false", dest=field.name, **kwargs)

which as far I tested does not break anything as the result should be identical. But it has the changed bonus that the argparser --help is less ambiguous as it would have defaults dataloader_pin_memory: True, no_dataloader_pin_memory: False.

[sgugger]

Let me double check, but that seems like a good change indeed. Thanks for explaining it to me!

[sgugger]

Mmm, actually it looks like changing this default to False changes the default value in the argparser: tried to laundh the script with and without --no_dataloader_pin_memory and printed the value of training_args.dataloader_pin_memory. Currently we get False and True respectively (as it should).
With the changed of default you are suggesting, I always get False.

[BramVanroy]

The reason that it is False is because of the order of the arguments. The no_ variant is added to the argparser first (before the actual argument), therefore its defaults will get precedence down the line. I can make a suggestion in a PR to move things around?

That would involve moving this line

transformers/src/transformers/hf_argparser.py

Lines 112 to 113 in 3a8de58

	if field.default is True:
	parser.add_argument(f"--no_{field.name}", action="store_false", dest=field.name, **kwargs)

to after this line

transformers/src/transformers/hf_argparser.py

Line 146 in 3a8de58

parser.add_argument(field_name, **kwargs)

It is not visually as pleasing to repeat the if-clause but I'd argue that it could be worth it when documented well enough.

[sgugger]

Oh the code of HfArgumentParser is not visually pleasing so that's not a problem ;-)
If you can suggest a PR, I'll test on the branch that everything is good with it.