Opened 16 years ago
Last modified 9 years ago
#1490 enhancement new
Allow twistd to "run" packages, e.g. 'twistd run mypackage --port 8080'
Description (last modified by )
Attachments (2)
Change History (63)
comment:1 Changed 16 years ago by
comment:2 Changed 16 years ago by
In this example, how would the 'mantissa' (axiom, actually) loader change the default location of pid and log files? because it needs to. The other problem with not making twistd into a library is that people don't want to type long command lines; your command-name is part of your UI. "axiomatic mantissa; axiomatic start -n" is bad enough, I don't think that "axiomatic mantissa; twistd -n --loader=mantissa mantissa.axiom" is really a step up.
comment:3 Changed 16 years ago by
I don't think long command line options are a significant problem. However, how about: == Option C == Can be implemented as A or B, but the Options is a subclass of the standard twistd Options class (or something, that won't actually work in practice). First argument is the loader plugin name/module depending on whether this is based on A or B. Example: axiomatic mantissa; twistd axiom -n # can do same magic that "axiomatic start" does to find mantissa.axiom vs. axiomatic mantissa; axiomatic start -n # this is actually longer to type, HOORJ
comment:4 Changed 16 years ago by
James suggets the simplification for versions A and B: instead of having a usage.Options instance, the callable accepts a list of strings, the remaining command-line options, allowing people to use their own command-line parser if they wish.
comment:5 Changed 16 years ago by
Changed title to reflect the current goal.
comment:6 Changed 16 years ago by
Actually, let me summarize latest goal again:
twistd <somepackage> [args]
Where <somepackage> is an importable package, e.g. "twisted.web" or "quotient". The code will be something like:
import somepackage svc = somepackage.makeService(nontwistdargs) runService(svc)
E.g.
$ twistd twisted.web2 --path=/tmp --port=8080 $ twistd someapp --config=my.config
comment:7 Changed 16 years ago by
comment:8 follow-up: 12 Changed 16 years ago by
comment:9 Changed 16 years ago by
To clarify, I am proposing something like using module name (e.g. "twisted.web", "twisted.protocols.ftp") as alternative way to finding the tap module interface used by mktap and twistd. That module would be expected to support thr tap module interface.
comment:10 Changed 16 years ago by
trac doesn't version the description field. editing it destroys information :(
comment:11 Changed 16 years ago by
Difficulties with a fully-qualified Python name approach:
- More typing
- No way to list all possible values
- Confusing to users - why does twisted.web2 work, but twisted.internet doesn't?
- Confusing to implementors - why do some parts of Twisted use plugins and others use bare names?
- Using names like twisted.web2 means putting code into twisted/web2/init.py - at least to import makeService from somewhere else
Advantages:
- Simpler application level API than plugins
- Potentially less work to do at runtime (although I'm not certain about this)
comment:12 Changed 16 years ago by
Replying to itamarst: He meant to put this:
#1922 partially makes this irrelevant, but still... why *do* we require plugins for mktap? Why not a Python module path? Especially given that we no longer persist the application, which means non-stable module references is less of a problem.
in the comment field, not the description field (which was blank before).
comment:13 Changed 16 years ago by
More typing seems not too important, especially most packages won't work via subpackages I suspect so it'll be same length (and same value probably); twisted is an exception. No way to list all values is a valid argument. I'm not sure having code in init.py is a big deal.
As far as confusing implementors, this seems way much much simpler. Installing a package for my proposal would be easier since you don't have to figure out how to install twisted/plugins/ (do we have documented distutils extensions for that?). The confusion would just come from Twisted, and if we go down this route maybe current plugins should be converted to use this method and leave plugin support only for backwards compat.
Not being able to list the available services does seem to be the main problem. BTW, does the new twistd have an option for listing them?
comment:14 Changed 16 years ago by
It indeed has an option for listing them. "--help".
comment:15 Changed 16 years ago by
Summarizing consensus achieved in last night's meeting:
twistd pluginnamewill be kept and gently encouraged
twistd runwill be a Twisted-provided plugin which takes one additional argument, the FQPN of an object providing IServiceMaker
- The introductory documentation will explain
twistd runfirst. A separate document will explain the steps for converting an application written in this style to an application written in the plugin style.
- The
runplugin will be written in a way so as to be useful as an example for writing
twistdplugins.
radix is working in this area, it seems, so I'm giving this ticket to him for the time being.
comment:16 Changed 15 years ago by
Changed 15 years ago by
comment:17 Changed 15 years ago by
For fun and profit, I did a quick hack that seems to accomplish that.
comment:18 Changed 14 years ago by
hey therve, write a unit test or fifteen for the code you wrote
comment:19 Changed 14 years ago by
comment:20 Changed 14 years ago by
I added the tests. You take care of the documentation ? :)
comment:21 Changed 14 years ago by
Why do we need to support both 'options' and 'Options'?
I've improved the branch a slight bit. I'll try to get to howto hacking soon, but 8.0.2 takes priority.
comment:22 Changed 14 years ago by
comment:23 Changed 14 years ago by
I supported Options so that 'twistd run twisted.web.tap' worked. Maybe this is not mandatory, though.
comment:24 Changed 14 years ago by
comment:25 Changed 14 years ago by
Glad you're working on this. I do still prefer the original idea where it would look for the service factory on the module level, so you could do:
twistd run myproject.service --port 8080
rather than
twistd run myproject.service.serviceMaker --port 8080
though I suppose myproject/init.py could import serviceMaker and give it a shorter name... and actually I guess a module could provide the interface. So never mind.
comment:26 Changed 14 years ago by
I have added some documentation to explain how to do what itamar suggested. I think this is ready to review, even though that should not be in 8.1. Every feedback is appreciated, especially on documentation.
comment:27 Changed 14 years ago by
- doc/core/howto/tap.xhtml
- the first paragraph about twistd run would probably benefit from a bit more markup - twistd run, options attribute, makeService method, twisted/plugins directory, fill service.py.
- In the 3rd sentence in that paragraph, it just need an options should be it just needs an options.
- I might reverse the order of the examples, first introducing the most simple way to implement the interface, then mentioning that if it is necessary to keep some state or if it is desired to have multiple services which are very similar but not exactly the same (ie, something multiple instances of one class providing the interface could provide) then bringing in the slightly more complicated example involving a class. This keeps the number of ideas a newbie encounters to an absolute minimum while providing them with something they can actually use. Then when they learn the more complicated way, they already know something, so they just need to adjust their understanding a bit to get the more complex idea. Likewise, this idea would suggest that twistd run should be introduced before the more complicated plugin-based system (which currently appears before this section in the tap howto). This also provides what might be another advantage - the simpler code can be expanded slightly to turn it into a plugin-style service, instead of starting over from scratch. Of course, deciding what approach will best convey information to someone trying to learn is very difficult, and I could be completely wrong about this change being an improvement on what's there already.
- Maybe it's worth mentioning that services written this way can't be discovered automatically, unlike those implemented via plugins? I don't know if that will surprise anyone. Alternatively, if twistd run is introduced first, then this can be skipped and later when the plugin-style is introduced, enumerability can be explained as a feature it has which goes beyond twistd run.
- twisted/application/app.py
ISimpleServiceMakerdoesn't seem like the best name. Unfortunately it seems like the existing
IServiceMakerforces this a bit. I guess I'm going to keep thinking about this a bit, and maybe I'll have a suggestion to make later on.
- I would leave evolved out of the docstring of
IServiceMakerand just describe the functional difference (ie, that it is discoverable as a plugin by twistd, which you have basically already explained there).
- twisted/test/test_twistd.py
twisted_runimport should be at the top of the file
- name
TestOptions,
TestServiceMaker,
testServiceMaker,
TestTap, and
testTapwith the
Stubor
Dummyor
Fakeprefix instead. Also, TAP is an acronym, so should be capitalized in those names.
- maybe
testServiceand
testTapcould be created in the first place by
setUpand deleted entirely in
tearDown, instead of re-using and cleaning the same instances?
test_unknowOptionsis missing a letter
- there should be a test that run is a subcommand of
twistd(ie, that the run plugin is found by getPlugins). it might also be nice to use
verifyObjecton the
RunOptionsinstance here.
comment:28 Changed 14 years ago by
Thanks for the comments. I tried to improve the documentation per your suggestions, I hope it's going in the right direction. Every else is good, except the
ISimpleServiceMaker name :)
comment:29 Changed 14 years ago by
Changed 14 years ago by
comment:30 Changed 14 years ago by
I attached some doc changes that might be good. It'd be nice to have some other eyes on this too.
The only other code change I'd suggest now is that we might not want to add a new file for
RunPlugin. Really, all of core's plugins could be in one file (probably one that imported a few names from elsewhere and didn't actually define anything itself). We seem to have grown quite a few extra files for core in
twisted/plugins. Maybe consolidating them is part of a different ticket, though.
comment:31 Changed 14 years ago by
There has been lots of conversation recently among exarkun, glyph and I about simplified deployment APIs. Given that twistd run is all about simplifying deployment, I think it's worth dumping here.
One point is that #3270 has a branch which includes a new 'react' function which helps in the deployment of programs which have a main function that takes a reactor and returns a Deferred. The program will exit when the Deferred fires.
This is an important use case that we should definitely take into consideration for simplified deployment, and I think at the least 'twistd run' should support something like it. I don't think 'react' should go in as-is because it's Yet Another deployment API which has severe limitations (for example, you can take advantage of common reactor choosing and logging code) and if we're going to be putting something into example code we should make sure it's actually something we want people to use.
Glyph also has some code that he called 'mainpoints' which allows you to decorate your main function and allow it to be used both on the command line and via a twistd plugin. I think this is closer to what I want, but I think it needs more consideration and tweaking.
Given all this interesting discussion of deployment APIs, I am slightly wary of having *this* ticket's branch merged, because I really don't want to introduce so many incompatible, limited deployment APIs all at once and make the "twisted deployment" scene even more complicated and horrible than it already is. I think we should have some discussions for weighing pros/cons of all of these different deployment APIs and at least understand what they are before blessing them with presence in trunk.
comment:32 Changed 14 years ago by
Back to back.
comment:33 Changed 14 years ago by
I didn't look at the doc changes, since ISTR that I wrote some of them, so someone else should probably take a look at those.
The command line would probably benefit from some different handling of the case where an FQPN which doesn't exist or which doesn't name an
ISimpleServiceMaker provider is given, showing something other than a Python traceback.
comment:34 Changed 14 years ago by
comment:35 Changed 14 years ago by
I explicitly added check for the interface in the plugin. I've added moduleProvides on some tap module, I'll add it to all if it's an accepted solution. Back to review.
comment:36 Changed 14 years ago by
Looks pretty good.
I dunno about the
moduleProvides in the existing tap modules. There's already a way to run all of them. Does this really add much of value? I'm not really strongly against it. If we decide to do it, there should be tests, though.
Some of the tests could use
TestCase.patch to be a little nicer, if you want. Definitely not a big deal.
I guess I did some bad things to the docs. There shouldn't be whitespace inside
<code class="API"></code> tags, it messes up the generated links.
tap.xhtml has this in a couple places now.
The remaining issues seem very minor now. Maybe we can get some other people involved in the discussion about having existing tap plugins also provide
ISimpleServiceMaker to resolve that question.
comment:37 Changed 14 years ago by
comment:38 Changed 14 years ago by
So I removed the tap support, use patch when I saw a use, and fix API link I think. Put it into review so that glyph can take a look at docs.
comment:39 Changed 14 years ago by
I think this is too complex. The original explanation of the requirements is somewhat muddled, because it uses large, established packages like "twisted.web" and "quotient" as examples. There's really no reason for these packages to use 'twistd run'; after all, they already provide plugins. From a user-experience perspective, plugins are better in every way; they're discoverable, they are self-documenting.
The real goal of a plugin like 'twistd run' is to serve as a sort of analogue to running 'python' on a simple little script to try something out. Right now, if you want to run a quick Twisted example, you have a few options.
- You can write a plugin, but that is a lot of work: you have to add an entry to sys.path, write a bunch of classes to satisfy the plugin interface, and so on.
- You can write a
.tacfile, but then you have no way to handle command-line arguments - or any other form of input, for that matter - so you have to keep modifying your source code if you (for example) want to run your example as a client against different servers.
- You can call
reactor.run()at the end of your script, but then you lose logging, daemonization, the ability to quickly reconfigure which reactor is being used (and so on, for all the features of twistd).
This is especially problematic for documentation examples, which want to focus on what the application code is doing, not on how to set up the reactor, or logging, or handle command-line arguments. Our documentation examples should be written to run with
twistd run.
twistd run should give you the best of all of these worlds. It should be as little work as just calling
listenTCP() /
reactor.run() at the end of a Python script; it should let you handle your command-line arguments (ideally however you want, involving
twisted.python.usage is often more baggage than it's worth if you have a command-line that just takes one argument), and it should provide you with the many splendors of
twistd.
A good litmus test for this is the documentation. The documentation should begin with an example where the user has to do no more than write a single python file. They should not need to modify their sys.path, and ideally they should not need to import anything beyond the core Twisted features required for their application code. This should work on either module or file names; I might have a module in my pre-existing project that I want to quickly run, or I might have a new file with no associated dependencies.
The documentation does need two parts though: writing a module that can be run by 'twistd run', and then evolving that module into a plugin, where it can be discovered and documented and so on.
I believe an ideal interface for this would be for the module in question to have a 'run' method defined in your module which takes a list of arguments and returns a service; even nicer would be to allow for returning a Deferred too, to make it easy to write examples which run, demonstrate something, and then terminate gracefully without needing to call
reactor.stop() in examples (and giving the misleading impression that it's normal to terminate the reactor after your application has finished doing something).
comment:40 follow-up: 41 Changed 14 years ago by
A run method that returns a service? For serious?
comment:41 Changed 14 years ago by
comment:42 Changed 12 years ago by
comment:43 Changed 11 years ago by
comment:44 follow-up: don't know if the branch currently does so, but I would suggest replacing .tac documentation with this, assuming #638 (and perhaps the proposed extra ticket for logging options) has been merged.
comment.
Woohoo design review.'m almost on board with this. I think that we should also have some facility to run a file, rather than a module. The use-case is that of small examples; thinking of everything in terms of fully-qualfiied module names will cause problems for people who don't yet understand how Python's module system works (i.e. almost everyone learning Twisted). The biggest conundrum this raises for me is that if one is to use a module, then having a function within that module makes sense; but if we want
twistd run filename.py to work, that may imply a script without a main function. Should these just be different plugins?
The second, much less important point: why the reactor and not the other global state (for example, everybody's favorite: logging) that the twistd command line sets up?
comment:46 Changed 11 years ago by
Re logging, the current proposal is to use the twistd command line for that, so I'm not sure if there's any point having the user program override it (the user program can always *add* another observer). I'm open to counterarguments though.
I want reactor in the argument list since I'm tired of answering questions about the difference between TCPService and reactor.listenTCP :) Plus we want explicit reactors everywhere.
As far as running files as opposed to modules, that's a good use case. Especially since would make running quick scripts with twistd much easier, which is a plus. I'm tempted to suggest something horrible involving overriding sys.argv, setting
__name__ to 'main' and turning reactor.run() into a noop or something, thereby letting you run most non-twistd twisted programs... but that would be horrible, so I won't.
How about
main(reactor, argv) as an entry point? Also I realized the returned Deferred could in theory let us implement the oft requested ability to determine the exit code.
comment:47 Changed 11 years ago by
I'm not totally against itamar's most recent proposal, but I think I would prefer that 'run' just use exactly the same API as the twistd plugin system (so 'twistd run mypackage' would just call 'mypackage.twistd_run()' and treat the result as it would treat the twistd plugin). If everyone else thinks this is a bad idea, though, then fine.
However, one thing that I really think is that we should NOT require the user to type in the FQPN of the function to call; that's just way too programmery and every time I use some third-party thing that makes me type the fully qualified names of, e.g., java classes, I puke a little. I'd much rather have 'run' look up a well-defined name on the object specified on the command line.
I have some time today and probably in the next couple of days to work on this.
comment:48 Changed 11 years ago by
comment:49 Changed 11 years ago by
If it's similar interface to plugins then therve's branch basically implements that, so not much work left to do. The key is agreeing on the API. How about a compromise?
"twistd run mypackage.module ..." looks at mypackage.module for object named "main" and calls it. We provide and document a utility function that can use a ISimpleServiceMaker (the plugin interface, basically).
This means you can do files (glyph's use case), simple functions, but also plugin-like things with services. So progressing from quick hack to full plugin can be done in stages, and this will be reflected in the documentation as well.
comment:50 follow-up::
def main(reactor, argv): reactor.listenTCP(int(argv[0]), MyServer()) or def main(reactor, argv): return TCPServer(int(argv[0]), MyServer()) or main = HeliocideHelper(ServiceMaker("myproject.tap", "whatever" ...))
comment:
I think that this API works fine if we want to support the second use-case as well. But we can defer that to a separate ticket.
Also, just to make sure I understand properly, the API is that you put some code into a module, 'foo.py', and then you 'twistd run foo', which looks up a function in 'foo' with a predetermined name (we think 'foo' is pretty good). Said function will be called with the reactor and the arguments passed to 'run'. If the module looks like this:
def main(reactor, argv): reactor.listenTCP(int(argv[0]), MyServer())
i.e. the function returns
None, then it will simply be called by
privilegedStartService of a special service and the reactor will run on its merry way.
If the function looks like this:
def main(reactor, argv): return TCPServer(int(argv[0]), MyServer())
i.e. the function returns an
IService, then it will be called by
privilegedStartService of a special service, which will notice that it is an
IService, and insert it into the service hierarchy so that
privilegedStartService,
startService, et. al. will be called on it as expected.
If it instead looks like this:
main = RunServiceMakerHelper(ServiceMaker("myproject.tap", "whatever" ...))
(
HeliocideHelper was not a useful name outside the context of the IRC discussion.)
RunServiceMakerHelper returns a callable object which will take the reactor arglist and do what
ServiceMaker normally does in a plugin; i.e. build an
IService from command-line arguments using a
usage.Options subclass. The tapname will be ignored, of course. Note that
--help (and maybe one or two other features of
Options?) may need to be special cased by the
run subcommand to make sure that it can make it into the arglist.
comment:52 Changed 11 years ago by
We can maintain this API, but your implementations suggestions won't work, glyph.
We can't run 'main' from privilegedStartService, because that introduces two problems:
- main won't be able to print any text to stdout
- main won't be able to exit gracefully.
This is all because privilegedStartService happens after daemonization. These features are pretty necessary if we want runnable modules to print help about their arguments and exit when not enough or bad arguments are provided.
So right now the implementation simply calls main() directly in RunPlugin.makeService(), which happens before daemonization and gives the main a chance to do those things. This also makes the main-returning-Service case simpler to implement, because RunPlugin.makeService can just return the service that main returned.
Are there any foreseeable problems with this strategy?
comment:53 Changed 11 years ago by
comment:54 Changed 10 years ago by
comment:55 Changed 10 years ago by
comment:56 Changed 10 years ago by
- As exarkun notes (implicitly), we perhaps should do something compatible with #3270, allowing an easy transition from simple scripts to twistd. The only inelegant thing about that is that if you no exit condition, you have to return a
Deferredyou drop on the floor. Perhaps #3270 should be extended before the next release when it crystallizes so that returning None means "don't exit?".
- Service running could be done using Glyph's suggestion of
twisted.application.service.runWithReactor(AFAIK no ticket?). If we go that route, we can just descope services from this ticket.
- Running before daemonization is a problem. For example, thread pools started before the fork won't survive daemonization. So given wanting to be compatible with
task.reactor, the code needs to run after the fork (i.e. startService).
comment:57 Changed 10 years ago by
Urgh.
If we want to be able to respond to bad command-line arguments *before* forking, when you still have stderr/stdout, command-line parsing code has to be done as root. But then you hit the problems with e.g. thread pools which as I said break if you naively create them before daemonizaton.
comment:58 Changed 10 years ago by
s/as root/before forking/.
comment:59 Changed 10 years ago by
If we want to be able to respond to bad command-line arguments *before* forking, when you still have stderr/stdout,
Another idea is that you don't have to throw away stderr/stdout until you've finished setup and won't have any more startup errors to report. There's another ticket somewhere that talks about this feature (I don't think I'll try to find it right now (hell, it may even be this ticket, there's enough history on it that I can't easily grasp it all)). There are a number of possible approaches to this. One is to just preserve the files across the various process operations until you don't need them any more. Another is to leave the parent running with a communication channel to all of its children until startup is done, and make the parent report any problems its children communicate back to it, and only exit after everything is working fine.