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

fix type annotation for debug arg #24033

Merged
merged 3 commits into from
Jun 21, 2023
Merged

fix type annotation for debug arg #24033

merged 3 commits into from
Jun 21, 2023

Conversation

Bearnardd
Copy link
Contributor

@Bearnardd Bearnardd commented Jun 5, 2023

What does this PR do?

Fix type annotation for debug argument in training_args.py

Fixes #23958

Who can review?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 5, 2023

The documentation is not available anymore as the PR was closed or merged.

@Bearnardd Bearnardd changed the title fix type annotation for debug arg [WIP] fix type annotation for debug arg Jun 5, 2023
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

It seems it's introduced an issue for the examples tests and is throwing TypeError. Once that's been resolved, ping and I'll review again. Lets us know if you have any questions or issues in the meantime.

@Bearnardd
Copy link
Contributor Author

Hi @amyeroberts, I have made a fix to the code. I'm not certain if there is a more concise way to address this, but I added an additional check for self.debug being None. The reason for this is that when using Union[str, List[DebugOption]], even if default="" is specified, it is still evaluated as None.

@Bearnardd Bearnardd changed the title [WIP] fix type annotation for debug arg fix type annotation for debug arg Jun 12, 2023
@amyeroberts
Copy link
Collaborator

@Bearnardd Thanks for updating. Could you share a snippet to reproduce showing the evaluation of debug as None if left as default?

If I create the training arguments directly, when working from main, debug defaults to [] with the type changes e.g.

In [1]: from transformers import TrainingArguments

In [2]: args = TrainingArguments("dummy_dir")

In [3]: args.debug
Out[3]: []

So it might be an environment or how it's being used in a script thing?

@Bearnardd
Copy link
Contributor Author

Hi @amyeroberts! It was one od the failing test cases. I will be back at home tomorrow so I will check that to confirm :)

@Bearnardd
Copy link
Contributor Author

Hi @amyeroberts! I have done some quick debugging. I was able to obtain the same results as you while running your snippet. However the problem appears when you try to run things from CLI. One of the test cases that were failing is test_run_seq2seq_no_dist from transformers/tests/extended/test_trainer_ext.py which uses command line arguments. In a result of running this test case there is a chain of internal function calls as follows:

parser = HfArgumentParser((ModelArguments, DataTrainingArguments, Seq2SeqTrainingArguments))
model_args, data_args, training_args = parser.parse_args_into_dataclasses()

parse_args_into_dataclasses underneath calls self.parse_known_args(args=args) which is a method derived from ArgumentParser.

namespace, remaining_args = self.parse_known_args(args=args) # hf_argparser.py:339

In the argparse itself there is a default action --debug which is initialize as None. And here is a trick: if debug argument is of type str then argparse is able to internally cast it into empty string however it leaves if as None if it is of type Union[str, List[DebugOption]. Thats why this test fails if we change type annotation of debug argument.

Is this explanation understandable for you or do you need some additional context/information :) ?

@amyeroberts
Copy link
Collaborator

@Bearnardd Thanks for such a detailed investigation and write up! In this case, resolving this with the --debug flag in argparse would be very involved and this None check works well :)

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

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.

Incorrect typing of debug field in TrainingArguments
3 participants