Merge pull request #1917 from davidmarin/thats-not-my-opt

warn about command-line opts for the wrong runner (fixes #1898)

mrjob/launch.py

@@ -26,6 +26,7 @@
 
 from mrjob.conf import combine_dicts
 from mrjob.conf import combine_lists
+from mrjob.options import _RUNNER_OPTS
 from mrjob.options import _add_basic_args
 from mrjob.options import _add_job_args
 from mrjob.options import _add_runner_args
@@ -480,7 +481,8 @@ def _runner_kwargs(self):
         # magic to the runner
         return combine_dicts(
             self._non_option_kwargs(),
-            self._kwargs_from_switches(self._runner_opt_names()),
+            # don't screen out irrelevant opts (see #1898)
+            self._kwargs_from_switches(set(_RUNNER_OPTS)),
             self._job_kwargs(),
         )
 

mrjob/runner.py

@@ -414,7 +414,6 @@ def _fix_opts(self, opts, source=None):
         results = {}
 
         for k, v in sorted(opts.items()):
-
             # rewrite deprecated aliases
             if k in deprecated_aliases:
                 if v is None:  # don't care
@@ -433,7 +432,7 @@ def _fix_opts(self, opts, source=None):
 
             if k in self.OPT_NAMES:
                 results[k] = None if v is None else self._fix_opt(k, v, source)
-            else:
+            elif v:
                 log.warning('Unexpected option %s (from %s)' % (k, source))
 
         return results

tests/test_job.py

@@ -29,6 +29,7 @@
 from mrjob.job import MRJob
 from mrjob.job import UsageError
 from mrjob.job import _im_func
+from mrjob.options import _RUNNER_OPTS
 from mrjob.parse import parse_mr_job_stderr
 from mrjob.protocol import BytesValueProtocol
 from mrjob.protocol import JSONProtocol
@@ -1355,7 +1356,7 @@ def _test_runner_kwargs(self, runner_alias):
         self.assertEqual(
             option_names,
             # libjars can be set by the job
-            (runner_class.OPT_NAMES -
+            (set(_RUNNER_OPTS) -
              self.CONF_ONLY_OPTIONS)
         )
 

tests/test_runner.py

@@ -547,7 +547,7 @@ def test_archive_remote_data(self):
 
 class ConfigFilesTestCase(SandboxedTestCase):
 
-    MRJOB_CONF_CONTENTS = None  # don't patch load_opts_from_mrjob_confsq
+    MRJOB_CONF_CONTENTS = None  # don't patch load_opts_from_mrjob_confs
 
     def save_conf(self, name, conf):
         conf_path = os.path.join(self.tmp_dir, name)
@@ -1108,3 +1108,53 @@ def test_malformed_step(self):
         steps = [dict(foo='bar')]
 
         self.assertRaises(NotImplementedError, EMRJobRunner, steps=steps)
+
+
+class UnexpectedOptsWarningTestCase(SandboxedTestCase):
+
+    MRJOB_CONF_CONTENTS = None  # don't patch load_opts_from_mrjob_confs
+
+    def setUp(self):
+        super(UnexpectedOptsWarningTestCase, self).setUp()
+
+        self.log = self.start(patch('mrjob.runner.log'))
+
+    def test_no_warning_by_default(self):
+        job = MRTwoStepJob(['-r', 'local', '--no-conf'])
+        job.sandbox()
+
+        with job.make_runner() as runner:
+            self.assertFalse(self.log.warning.called)
+
+    def test_unexpected_opt_from_mrjob_conf(self):
+        conf_path = self.makefile('mrjob.custom.conf')
+
+        with open(conf_path, 'w') as f:
+            dump_mrjob_conf(
+                dict(runners=dict(local=dict(land='useless_swamp'))), f)
+
+        job = MRTwoStepJob(['-r', 'local', '-c', conf_path])
+        job.sandbox()
+
+        with job.make_runner() as runner:
+            self.assertTrue(self.log.warning.called)
+            warnings = '\n'.join(
+                arg[0][0] for arg in self.log.warning.call_args_list)
+
+            self.assertIn('Unexpected option', warnings)
+            self.assertIn('land', warnings)
+            self.assertIn(conf_path, warnings)
+
+    def test_unexpected_opt_from_command_line(self):
+        # regression test for #1898. local runner doesn't support *zone*
+        job = MRTwoStepJob(['-r', 'local', '--no-conf', '--zone', 'DANGER'])
+        job.sandbox()
+
+        with job.make_runner() as runner:
+            self.assertTrue(self.log.warning.called)
+            warnings = '\n'.join(
+                arg[0][0] for arg in self.log.warning.call_args_list)
+
+            self.assertIn('Unexpected option', warnings)
+            self.assertIn('zone', warnings)
+            self.assertIn('command line', warnings)