Allow using fire without editing code directly. #110

jtratner · 2018-01-03T03:02:54Z

Enable python -m fire <module> <args> to work - closes #29. @dbieber

I wanted to restart the discussion in #29 with a very simple implementation that doesn't lock us into any advanced functionality at all. Everything else works exactly the same, except that you prefix with python -m fire and the first argument must resolve to a module.

`python -m fire` vs. `fire`

I used python -m fire rather than an external fire tool, because it's really important that the python interpreter be exactly the same, and this avoids confusing bugs where fire resolves to something in the global environment, but you're trying to test something in a virtualenv or conda environment.

Testing

I need to think a little bit about how to test running fire with -m (I'm sure someone on the internet knows) but I'd like to get feedback on whether this a good implementation before investing too much time there.

"Bugs"

--interactive currently jumps you into a shell with the specified module loaded, rather than what you might expect (from <module> import *)

Examples

$ python -m fire tempfile mkdtemp
/var/folders/fm/sjgpzb856kld04jvq82bxrcw0000gp/T/tmpCbfg_1

or

$ python -m fire urllib unquote genetics%3Dawesome%26editor%3Dcrispr
genetics=awesome&editor=crispr

This follows normal python import semantics, so if you have a script
file, it'll work if named just as a module:

# myscript.py
def fn1(x, y):
    return x

$ python -m fire myscript 5 6
5

Why manipulate sys.argv?

when running with -m, nothing else is going to be pulling from sys.argv
it makes for a nicer help text (rather than __main__.py you can show the module)

›› python -m fire base64
Type:        module
String form: <module 'base64' from '/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/base64.pyc'>
File:        /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/base64.py
Line:        1
Docstring:   RFC 3548: Base16, Base32, Base64 Data Encodings

Usage:       base64
             base64 EMPTYSTRING
             base64 MAXBINSIZE
             base64 MAXLINESIZE
             base64 b16decode
             base64 b16encode
             base64 b32decode
             base64 b32encode
             base64 b64decode
             base64 b64encode
             base64 binascii
             base64 decode
             base64 decodestring
             base64 encode
             base64 encodestring
             base64 k
             base64 re
             base64 standard-b64decode
             base64 standard-b64encode
             base64 string
             base64 struct
             base64 test
             base64 test1
             base64 urlsafe-b64decode
             base64 urlsafe-b64encode
             base64 v

jtratner · 2018-01-03T03:20:46Z

The lint failures are all unrelated (my only change is to add fire.__main__ and they all are other modules)

dbieber · 2018-01-04T14:34:16Z

Lint failures started unexpectedly recently, and I can confirm they're unrelated.

dbieber · 2018-01-04T14:47:45Z

fire/__main__.py

+  module = sys.argv[1]
+  module = importlib.import_module(module)
+  sys.argv = sys.argv[1:2] + sys.argv[2:]
+  fire.Fire(module)


Rather than manipulating sys.argv, you can pass the name= and command= arguments to fire.Fire.
name=module, command=sys.argv[2:].

jtratner · 2018-01-06T00:16:00Z

@dbieber - added some test cases

jtratner · 2018-01-06T00:18:53Z

fire/test_main.py

+  def test_name_setting(self):
+    # confirm one of the usage lines has tempfile gettempdir
+    # with self.assertRaisesFireExit(2, 'tempfile gettempdir'):
+    with self.assertOutputMatches('tempfile gettempdir'):


I'm confused by the behavior here - argparse raises SystemExit(0,) if you say -h and SystemExit(2,) if not enough arguments, so I don't understand here (where it's printing out a usage statement) why it doesn't exit either 0 or 2 at this point.

(I think we've discussed this before too, but maybe I'm forgetting).

Commented out line is how I think it should work, but what actually works is uncommented.

It's exiting with SystemExit(0), but not with a FireExit. Here are the current rules:

Whenever Fire encounters a FireError (this occurs when when a Fire command cannot be executed, and is different from user code raising an exception), Fire raises a FireExit with code 2 (Recall that FireExit is a subclass of SystemExit, and so the program exists).

When Fire is used with the help or trace flags, Fire raises a FireExit with code 0 if successful.

Otherwise (the default case), the program just ends, which is equivalent to a SystemExit(0), but not a FireExit

In the default case, Fire just returns (it doesn't raise the SystemExit explicitly). This allows a user to use the return value of the call to Fire if they want without having to catch an exception.

gotcha - I think I was expecting it to set a code of 2 just like argparse, but pretty bike-sheedy difference

jtratner · 2018-01-06T00:19:07Z

fire/__main__.py

+
+import fire
+
+def main(args=None):


did this to make it easier to test

dbieber · 2018-01-08T19:36:43Z

If you rebase, the lint errors are resolved now.

dbieber · 2018-01-08T20:09:27Z

fire/__main__.py

+def main(args):
+  module_name = args[1]
+  module = importlib.import_module(module_name)
+  fire.Fire(module, name=module_name, command=args[2:])


Do we want module or vars(module)?

I was hoping that vars(module) would make it so that interactive mode would look like from module import *, but that didn't work either.

My inclination would be to leave it as just module - seems like the inference should be for how python objects work by default, not for dictionaries (i.e., in terms of hiding "protected" attributes, etc).

Oh yeah, my bad. I was also making that suggestion hoping it would behave like "import *".
Leaving it as module sgtm.

dbieber · 2018-01-08T20:13:36Z

fire/test_main.py

+  def test_name_setting(self):
+    # confirm one of the usage lines has tempfile gettempdir
+    # with self.assertRaisesFireExit(2, 'tempfile gettempdir'):
+    with self.assertOutputMatches('tempfile gettempdir'):


It's exiting with SystemExit(0), but not with a FireExit. Here are the current rules:

Whenever Fire encounters a FireError (this occurs when when a Fire command cannot be executed, and is different from user code raising an exception), Fire raises a FireExit with code 2 (Recall that FireExit is a subclass of SystemExit, and so the program exists).

When Fire is used with the help or trace flags, Fire raises a FireExit with code 0 if successful.

Otherwise (the default case), the program just ends, which is equivalent to a SystemExit(0), but not a FireExit

In the default case, Fire just returns (it doesn't raise the SystemExit explicitly). This allows a user to use the return value of the call to Fire if they want without having to catch an exception.

dbieber · 2018-01-08T20:16:33Z

fire/test_main.py

+    with self.assertOutputMatches('%s\n' % expected):
+      __main__.main(['__main__.py', 'os.path', 'join', 'part1', 'part2', 'part3'])
+    with self.assertOutputMatches('%s\n' % expected):
+      __main__.main(['__Main__.py', 'os', 'path', '-', 'join', 'part1', 'part2', 'part3'])


Fix capitalization of '__main__.py'

Let users run fire as the main function to use fire to create CLIs around *any* python module. This allows you to wrap third party libraries with fire without writing any code! Exciting! :D E.g.: ``` $ python -m fire tempfile mkdtemp /var/folders/fm/sjgpzb856kld04jvq82bxrcw0000gp/T/tmpCbfg_1 ``` or ``` $ python -m fire urllib unquote genetics%3Dawesome%26editor%3Dcrispr genetics=awesome&editor=crispr ``` This follows normal python import semantics, so if you have a script file, it'll work if named just as a module: ``` # myscript.py def fn1(x, y): return x ``` ``` $ python -m fire myscript 5 6 5 ```

jtratner · 2018-01-09T16:17:13Z

@dbieber - I think this is ready for another round now - all the tests pass and I updated the test cases to reflect the description of fire's behavior.

dbieber

a handful of nits to satisfy the internal linter

dbieber · 2018-01-09T21:50:49Z

fire/test_main.py

+from fire import testutils
+
+class TestAsMainModule(testutils.BaseTestCase):
+  def test_name_setting(self):


nit: use testCamelCase for test names.

dbieber · 2018-01-09T21:51:21Z

fire/test_main.py

+from fire import __main__
+from fire import testutils
+
+class TestAsMainModule(testutils.BaseTestCase):


nit: "MainModuleTest"

dbieber · 2018-01-09T21:54:24Z

fire/__main__.py

+def main(args):
+  module_name = args[1]
+  module = importlib.import_module(module_name)
+  fire.Fire(module, name=module_name, command=args[2:])


Oh yeah, my bad. I was also making that suggestion hoping it would behave like "import *".
Leaving it as module sgtm.

dbieber · 2018-01-09T21:59:20Z

fire/__main__.py

@@ -0,0 +1,16 @@
+"""


Sorry to be so picky; have to satisfy our internal linter.
Docstring/header format should be one of:

"""One line docstring. Rest of docstring. """ from __future__ import absolute_import from __future__ import division from __future__ import print_function

or

"""One line docstring.""" from __future__ import absolute_import from __future__ import division from __future__ import print_function

dbieber · 2018-01-09T22:00:21Z

fire/test_main.py

+    # confirm one of the usage lines has tempfile gettempdir
+    with self.assertOutputMatches('tempfile gettempdir'):
+      __main__.main(['__main__.py', 'tempfile'])
+  def test_arg_passing(self):


nit: blank line before each "def" in the class, and two blank lines before the "class" line at line 9.

jtratner · 2018-01-10T03:42:04Z

okay @dbieber - I think this should fix all the lint issues you cited

jtratner · 2018-01-17T15:19:19Z

@dbieber - ping if you've got a moment :)

jtratner · 2018-02-23T17:59:18Z

@dbieber - any remaining steps to merge this? :)

…

On Tue, Jan 9, 2018 at 2:00 PM David Bieber ***@***.***> wrote: ***@***.**** commented on this pull request. a handful of nits to satisfy the internal linter ------------------------------ In fire/test_main.py <#110 (comment)>: > @@ -0,0 +1,21 @@ +""" +Test using fire via python -m fire +""" +import os + +from fire import __main__ +from fire import testutils + +class TestAsMainModule(testutils.BaseTestCase): + def test_name_setting(self): nit: use testCamelCase for test names. ------------------------------ In fire/test_main.py <#110 (comment)>: > @@ -0,0 +1,21 @@ +""" +Test using fire via python -m fire +""" +import os + +from fire import __main__ +from fire import testutils + +class TestAsMainModule(testutils.BaseTestCase): nit: "MainModuleTest" ------------------------------ In fire/__main__.py <#110 (comment)>: > @@ -0,0 +1,16 @@ +""" +Module for running python fire as a "main" function and thereby allowing using +fire with other programs. +""" +import importlib +import sys + +import fire + +def main(args): + module_name = args[1] + module = importlib.import_module(module_name) + fire.Fire(module, name=module_name, command=args[2:]) Oh yeah, my bad. I was also making that suggestion hoping it would behave like "import *". Leaving it as module sgtm. ------------------------------ In fire/__main__.py <#110 (comment)>: > @@ -0,0 +1,16 @@ +""" Sorry to be so picky; have to satisfy our internal linter. Docstring/header format should be one of: """One line docstring. Rest of docstring. """ from __future__ import absolute_import from __future__ import division from __future__ import print_function or """One line docstring.""" from __future__ import absolute_import from __future__ import division from __future__ import print_function ------------------------------ In fire/test_main.py <#110 (comment)>: > @@ -0,0 +1,21 @@ +""" +Test using fire via python -m fire +""" +import os + +from fire import __main__ +from fire import testutils + +class TestAsMainModule(testutils.BaseTestCase): + def test_name_setting(self): + # confirm one of the usage lines has tempfile gettempdir + with self.assertOutputMatches('tempfile gettempdir'): + __main__.main(['__main__.py', 'tempfile']) + def test_arg_passing(self): nit: blank line before each "def" in the class, and two blank lines before the "class" line at line 9. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#110 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhjq_5BIdDEVGeN6t2vcXuE8klqlTVaks5tI-GLgaJpZM4RRTcN> .

dbieber · 2018-02-23T18:13:23Z

Will take a look post-release. Apologies for missing your previous ping.

Let users run fire as the main function to use fire to create CLIs around *any* python module. This allows you to wrap third party libraries with fire without writing any code! Exciting! :D E.g.: ``` $ python -m fire tempfile mkdtemp /var/folders/fm/sjgpzb856kld04jvq82bxrcw0000gp/T/tmpCbfg_1 ``` or ``` $ python -m fire urllib unquote genetics%3Dawesome%26editor%3Dcrispr genetics=awesome&editor=crispr ``` This follows normal python import semantics, so if you have a script file, it'll work if named just as a module: ``` # myscript.py def fn1(x, y): return x ``` COPYBARA_INTEGRATE_REVIEW=#110 from jtratner:main-fire dbe70e2 PiperOrigin-RevId: 299450705 Change-Id: I9ebead46b141657d826ca81a787e15d6f8d9c9b8

dbieber · 2020-03-06T23:46:09Z

Alright, we're doing it! 598937e

We'll include this in the next release.

dbieber reviewed Jan 4, 2018

View reviewed changes

jtratner commented Jan 6, 2018

View reviewed changes

fire/__main__.py Outdated

import fire

def main(args=None):

Copy link

Contributor Author

jtratner Jan 6, 2018

Choose a reason for hiding this comment

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

did this to make it easier to test

jtratner force-pushed the main-fire branch from 66c5002 to c6dece8 Compare January 6, 2018 00:20

dbieber reviewed Jan 8, 2018

View reviewed changes

jtratner added 2 commits January 8, 2018 17:43

Add tests for main function

519044c

jtratner force-pushed the main-fire branch from c6dece8 to 519044c Compare January 9, 2018 01:44

jtratner added 4 commits January 8, 2018 18:17

Fix capitalization + lint

c70b423

Fix member attribute name and add additional check

810a004

Fix up test completely

3dc93a0

Remove leftover comment

405596e

dbieber reviewed Jan 9, 2018

View reviewed changes

jtratner added 2 commits January 9, 2018 19:39

More lint fixes

ce7ea6c

Lint fix

dbe70e2

nfultz mentioned this pull request Jul 24, 2018

WIP Adding __main__ script #131

Closed

dbieber force-pushed the master branch from ffbfa51 to 4695f34 Compare December 5, 2019 21:41

dbieber added the cla: yes Author has signed CLA label Feb 28, 2020

dbieber closed this Mar 6, 2020

This was referenced Mar 6, 2020

Use fire without editing code directly #29

Closed

Add python -m fire? #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using fire without editing code directly. #110

Allow using fire without editing code directly. #110

jtratner commented Jan 3, 2018

jtratner commented Jan 3, 2018

dbieber commented Jan 4, 2018 •

edited

dbieber Jan 4, 2018

jtratner commented Jan 6, 2018

jtratner Jan 6, 2018

dbieber Jan 8, 2018 •

edited

jtratner Jan 9, 2018

jtratner Jan 6, 2018

dbieber commented Jan 8, 2018

dbieber Jan 8, 2018

jtratner Jan 9, 2018

dbieber Jan 9, 2018

dbieber Jan 8, 2018 •

edited

dbieber Jan 8, 2018 •

edited

jtratner commented Jan 9, 2018

dbieber left a comment

dbieber Jan 9, 2018

dbieber Jan 9, 2018

dbieber Jan 9, 2018

dbieber Jan 9, 2018

dbieber Jan 9, 2018

jtratner commented Jan 10, 2018

jtratner commented Jan 17, 2018

jtratner commented Feb 23, 2018 via email

dbieber commented Feb 23, 2018

dbieber commented Mar 6, 2020

Allow using fire without editing code directly. #110

Allow using fire without editing code directly. #110

Conversation

jtratner commented Jan 3, 2018

python -m fire vs. fire

Testing

"Bugs"

Examples

Why manipulate sys.argv?

jtratner commented Jan 3, 2018

dbieber commented Jan 4, 2018 • edited

Choose a reason for hiding this comment

jtratner commented Jan 6, 2018

Choose a reason for hiding this comment

dbieber Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbieber commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbieber Jan 8, 2018 • edited

Choose a reason for hiding this comment

dbieber Jan 8, 2018 • edited

Choose a reason for hiding this comment

jtratner commented Jan 9, 2018

dbieber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtratner commented Jan 10, 2018

jtratner commented Jan 17, 2018

jtratner commented Feb 23, 2018 via email

dbieber commented Feb 23, 2018

dbieber commented Mar 6, 2020

`python -m fire` vs. `fire`

dbieber commented Jan 4, 2018 •

edited

dbieber Jan 8, 2018 •

edited

dbieber Jan 8, 2018 •

edited

dbieber Jan 8, 2018 •

edited