-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Write tests for calling CLI arguments downstream to ensure correctly-returned types #1518
Comments
In my opinion, raising proper errors in the main code (not the tests) is preferable to using asserts.
Proper exceptions can also be pytested: |
One downside of raising errors in the code ahead of merge to main is that you'd have to know ahead of runtime that the code you're adding to the command line args requires new error handling. But, if all of the arguments are checked in a pre-merge test, you wouldn't be able to run the package to begin with and merge the code. What we'd like to test is automatically adding new command line args that get parsed correctly downstream: As a specific case, in this case, this environment variable should be a if args.trust_remote_code:
os.environ["HF_DATASETS_TRUST_REMOTE_CODE"] = (
args.trust_remote_code if args.trust_remote_code else True
)
args.model_args = (
args.model_args
+ f",trust_remote_code={os.environ['HF_DATASETS_TRUST_REMOTE_CODE']}"
) Ultimately this causes an error in |
Maybe pydantic can help somehow
As defined in documentation |
Closed with #1566 |
Issue: Currently we don't assert that passed-in args are the correct type passed to downstream code, causing runtime exceptions. See example of issue here.
There's a couple approaches we can try here, from least effort to most as far as I see it, most manual to most automated:
assert
statements to all current arg-level actions incli_evaluate()
cli_evaluate()
cli_evaluate()
into likely into two methods, one that starts from the method signature, and one that starts at line 309, where the evaluation logger starts, so we can test the part of the method where we assign variables from the CLI directly, and unit test that,Happy to discuss any of these and open to other approaches if they've been previously discussed, and also happy to take this as an issue.
The text was updated successfully, but these errors were encountered: