Problem with modal argsparser flexibility

[simonrosenberg]

Read this PR (comments + diff) to understand the problem #394

Let's fix this by preserving backward compatibility

• remove --modal entirely from argparse
• remove --modal from config
• add --no-modal to argparser. No-modal allows to disable modal

Make sure the change is backward compatible.
Make sure to apply the change on all CLIs that have a modal parameter

[openhands-ai]

I'm on it! simonrosenberg can track my progress at all-hands.dev

[openhands-ai]

Summary

I have successfully fixed issue #396 by replacing --modal with --no-modal in the benchmark CLIs. The changes have been pushed and a PR has been created.

PR Link

#397

Changes Made

Requirement	Status
Remove --modal entirely from argparse	✅ Done
Remove --modal from config	✅ Done
Add --no-modal to argparser	✅ Done
Backward compatible	✅ Done
Apply to all CLIs with modal parameter	✅ Done
Add tests	✅ Done

Files Modified

1. benchmarks/swebench/config.py: Removed "modal": True from EVAL_DEFAULTS

2. benchmarks/swebench/eval_infer.py:

   • Replaced --modal argument with --no-modal (dest="modal", action="store_false")
   • Updated function signature to default modal=True
   • Updated examples in help text

3. benchmarks/swebenchmultimodal/eval_infer.py:

   • Added --no-modal argument
   • Added modal parameter to run_swebench_multimodal_evaluation function
   • Updated function call to pass modal argument

4. tests/test_modal_argparser.py (new): Added 9 comprehensive tests for:

   • Default modal value is True
   • --no-modal flag sets modal to False
   • --modal flag is not accepted
   • Config no longer contains modal
   • Backward compatibility of function signatures

Testing

All 41 tests pass, including the 9 new tests specifically for the modal argument changes.

View full conversation