Differential D3

extracting python module import/execution as a standalone directive. Fixes T76
ClosedPublic
Actions

Authored by tflink on Jan 24 2014, 6:40 PM.

Details

Reviewers
ralph
Maniphest Tasks
T76: create python directive from existing python loading code in runner
Commits
rLTRN60806d868414: extracting python module import/execution as a standalone directive. Fixes T76
Summary

just moving code to import and execute python code to a separate directive, not fixing any of the other issues in the runner

Test Plan

added several unit tests, have tested on my local system to make sure that the example tasks still run.

Diff Detail

Branch
feature/T76-python-directive-refactor
Lint
No Linters Available
Unit
No Unit Test Coverage
ralph added a comment.Jan 24 2014, 7:09 PM

Generally, this is cool; +1!

I just have a couple pedantic complaints in the source above.

libtaskotron/directives/python_directive.py
13

Just a note on terminology. In python, a method is *always* part of a class. If it is not part of a class then it is a function.

Is there a particular reason why you are disallowing methods-on-a-class? Could a user do something like this in their task?

class MyClass(object):
    def my_task(self):
        return 42

my_hidden_object = MyClass()

i_want_taskotron_to_execute_this = my_hidden_object.my_task

taskotron (I think) doesn't necessarily need to know that i_want_taskotron_to_execute_this is *really* a method on an object, although it is.

Similarly, a task writer might provide something like this:

class MyTaskClass(object):
    def __call__(self):
        return 42

i_want_taskotron_to_execute_this = MyTaskClass()

Then, taskotron would try to call i_want_tasktron_to_execute_this and (I think) succeed.. even though that thing is neither a function nor a method. It is a *callable*.

I think that's the best choice of language for the PythonDirective docs. Something like

This code must be part of a valid python module and must be a valid callable.

libtaskotron/runner.py
64–65

There is no need to keep the .keys() call here. The default iterator for a dict just returns the keys.

88

This line seems excessively long. Perhaps break it out for readability?

options = {'basedir': basedir, 'method_name': 'run', 'workingdir': self.workdir}
exec_output = helper.process(pyfilename, options, {})
tflink added a comment.Jan 24 2014, 7:44 PM

Thanks for the comments, will update to address concerns raised - especially around the function/method/callable stuff

libtaskotron/directives/python_directive.py
13

Thanks for the terminology correction. I was pretty sure that I was being inaccurate but wasn't quite sure what the right terminology was :)

The rationale was code simplicity and getting this done since folks are blocked for lack of examples. I didn't want to deal with conditionals on loading a class and then finding a method in that class right now. I wasn't planning on having the 'function only' restriction in the long term.

Now that I'm thinking about it, I'm not sure I see a whole lot of functional difference between what's written and what you're proposing. However, I prefer the way you're approaching it (referring to a callable instead of caring about function/method/class) - it's not really more complex and avoids the stated, but not-needed restriction on functions vs. methods.

libtaskotron/runner.py
88

Actually, the entirety of this logic is going away soon (I've already started that refactoring) as it's way too hard-coded but that's no excuse for poor quality code :)

tflink updated this revision.Jan 24 2014, 10:11 PM
  • changed funcional test names to functest for more differentiation and disabled running of functional tests by default
  • changed python directive around to work on callables instead of function names, cleaned up docs and added some tests
  • cleaned up some calls, not-needed calls to keys() and updated runner for changes in python directive
ralph accepted this revision.Jan 25 2014, 1:50 AM
tflink added a comment.Jan 26 2014, 12:08 AM

I want to get some docs around this code written, so I changed the reviewers/ccs to get going to merge it since it has been reviewed. If anyone who hasn't seen the code yet has concerns about it, please let me know.

tflink closed this revision.Jan 26 2014, 12:18 AM

Revision Contents

Diff 8

View Options

libtaskotron/directives/python_directive.py

  • This file was added.
1​import imp
2​import os
3​
4​from libtaskotron.directives import BaseDirective
5​
6​directive_class = 'PythonDirective'
7​
8​
9​class PythonDirective(BaseDirective):
10​ """The python directive is designed to execute a piece of python code as part
11​ of task execution. Given a callable name 'dothis_thing', possible implementations
12​ in the loaded code could include:
13​
ralphUnsubmitted
Not Done

Just a note on terminology. In python, a method is *always* part of a class. If it is not part of a class then it is a function.

Is there a particular reason why you are disallowing methods-on-a-class? Could a user do something like this in their task?

class MyClass(object):
    def my_task(self):
        return 42

my_hidden_object = MyClass()

i_want_taskotron_to_execute_this = my_hidden_object.my_task

taskotron (I think) doesn't necessarily need to know that i_want_taskotron_to_execute_this is *really* a method on an object, although it is.

Similarly, a task writer might provide something like this:

class MyTaskClass(object):
    def __call__(self):
        return 42

i_want_taskotron_to_execute_this = MyTaskClass()

Then, taskotron would try to call i_want_tasktron_to_execute_this and (I think) succeed.. even though that thing is neither a function nor a method. It is a *callable*.

I think that's the best choice of language for the PythonDirective docs. Something like

This code must be part of a valid python module and must be a valid callable.

tflinkAuthorUnsubmitted
Not Done

Thanks for the terminology correction. I was pretty sure that I was being inaccurate but wasn't quite sure what the right terminology was :)

The rationale was code simplicity and getting this done since folks are blocked for lack of examples. I didn't want to deal with conditionals on loading a class and then finding a method in that class right now. I wasn't planning on having the 'function only' restriction in the long term.

Now that I'm thinking about it, I'm not sure I see a whole lot of functional difference between what's written and what you're proposing. However, I prefer the way you're approaching it (referring to a callable instead of caring about function/method/class) - it's not really more complex and avoids the stated, but not-needed restriction on functions vs. methods.

14​ class TaskClass(object):
15​ def my_task(self):
16​ return "I'm a task class method!"
17​
18​ _instantiated_class = TaskClass()
19​
20​ task_class_target = _instantiated_class.my_task
21​
22​ In this case, you could pass in 'task_class_target' as the callable arg
23​
24​ class EmbeddedCallClass(object):
25​ def __call__(self):
26​ return "I'm a __call__ method in a class!"
27​
28​ embedded_task_target = EmbeddedCallClass()
29​
30​ In this case, you could pass in 'embedded_task_target' as the callable arg
31​
32​ def task_method():
33​ return "I'm a task method!"
34​
35​ And in this third case, you would pass in 'task_method' as the callable arg
36​
37​ format: "python: <python filename> callable=<method name> [<kwargname>=<kwargvalue>'
38​ required params: <python filename>, callable
39​ optional params: farther key/value pairs, to be passed into the callable as kwargs
40​ """
41​
42​ def _do_getattr(self, module, name):
43​ """Isolation of getattr to make the code more easily testable
44​
45​ @param module module from which to getattr from
46​ @param name name of object to getattr
47​ @returns object retrieved from module
48​ """
49​
50​ return getattr(module, name)
51​
52​ def execute(self, task_module, method_name, kwargs):
53​ """Execute a callable in the specified module
54​
55​ @param task_module module containing the specified callable
56​ @param method_name name of callable to execute
57​ @param kwargs kwargs to pass as parameters into the callable
58​ @return output from the executed method
59​ """
60​
61​ task_method = self._do_getattr(task_module, method_name)
62​
63​ output = task_method(**kwargs)
64​
65​ return output
66​
67​ def load_pyfile(self, filename):
68​ """Import python code specified
69​
70​ @param filename absolute path to the python file
71​ """
72​
73​ task_importname = 'runtask_%s' % os.path.basename(filename).rstrip('.py')
74​ task = imp.load_source(task_importname, filename)
75​
76​ return task_importname, task
77​
78​ def checkfile(self, filename):
79​ """Check to see if the file exists
80​
81​ @param filename abspath to the filename
82​ @throws Exception if the file does not exist
83​ """
84​
85​ if not os.path.exists(filename):
86​ raise Exception("The file %s does not exist!" % filename)
87​
88​ def process(self, command, input_data, env_data):
89​ """For the python directive, we're expecting a yaml declaration of the form
90​ "python: <python file> callable=<method name> [kwarg1=something ...]"
91​
92​ @param command full path to the python module
93​ @param input_data dictionary of all kwargs from yaml declaration, including method_name
94​ @param env_data information on the environment in which we're running
95​ @return output from the method specified, this is expected to be TAP output
96​ """
97​
98​ method_name = input_data.pop('callable')
99​ basedir = env_data['basedir']
100​
101​ pyfile = os.path.join(basedir, command)
102​
103​ self.checkfile(pyfile)
104​
105​ task_name, task = self.load_pyfile(pyfile)
106​
107​ output = self.execute(task, method_name, input_data)
108​
109​ return output
View Options

libtaskotron/runner.py

Show All 35 Lines25 def _load_directive(self, directive_name, directive_dir=None):
36 loaded_directive = imp.load_source(real_name, directive_file) 36​ loaded_directive = imp.load_source(real_name, directive_file)
37 self.directives[directive_name] = loaded_directive 37​ self.directives[directive_name] = loaded_directive
38 38​
39 39​
40 def do_prep(self): 40​ def do_prep(self):
41 commands = self.taskdata['preparation'] 41​ commands = self.taskdata['preparation']
42 42​
43 supported_prep = ['koji'] 43​ supported_prep = ['koji']
44 for command_type in commands.keys(): 44​ for command_type in commands:
45 if command_type in supported_prep: 45​ if command_type in supported_prep:
46 self._load_directive(command_type) 46​ self._load_directive(command_type)
47 else: 47​ else:
48 raise Exception("Prep command %s not currently supported" % command_type) 48​ raise Exception("Prep command %s not currently supported" % command_type)
49 49​
50 # the only supported prep right now is koji, makes for simple code :) 50​ # the only supported prep right now is koji, makes for simple code :)
51 51​
52 helper = self.directives['koji'].KojiDirective() 52​ helper = self.directives['koji'].KojiDirective()
53 53​
54 # instead, merge dict coming out of prep - let prep decide what data is named 54​ # instead, merge dict coming out of prep - let prep decide what data is named
55 prep_output = helper.process(commands['koji'], self.inputdata, {'tmpdir':self.workdir}) 55​ prep_output = helper.process(commands['koji'], self.inputdata, {'tmpdir':self.workdir})
56 self.working_data.update(prep_output) 56​ self.working_data.update(prep_output)
57 57​
58 def do_deps(self, deps): 58​ def do_deps(self, deps):
59 print "fake verifying deps: %s" % str(deps) 59​ print "fake verifying deps: %s" % str(deps)
60 60​
61 def do_execution(self): 61​ def do_execution(self):
62 commands = self.taskdata['execution'] 62​ commands = self.taskdata['execution']
63 print "doing execution with commands: %s" % str(commands) 63​ print "doing execution with commands: %s" % str(commands)
64 64​
65 supported_prep = ['python'] 65​ supported_execution = ['python']
ralphUnsubmitted
Not Done

There is no need to keep the .keys() call here. The default iterator for a dict just returns the keys.

66 for command_type in commands.keys(): 66​
67 if command_type not in supported_prep: 67​ for command_type in commands:
68 raise Exception("Prep command %s not currently supported" % command_type) 68​ if command_type in supported_execution:
69​ self._load_directive(command_type)
70​ else:
71​ raise Exception("Execution command %s not currently supported" % command_type)
69 72​
70 # the only supported execution right now is rpmlint as a python file 73​ # the only supported execution right now is rpmlint as a python file
71 args = commands['python'].split() 74​ args = commands['python'].split()
72 75​
73 # for now, ignore any args since we're just supporting pylint 76​ # for now, ignore any args since we're just supporting pylint
74 pyfilename = args[0] 77​ pyfilename = args[0]
75 78​
76 basedir = os.path.dirname(os.path.abspath(self.inputdata['taskfile'])) 79​ basedir = os.path.dirname(os.path.abspath(self.inputdata['taskfile']))
77 80​
78 #import the task module and execute run() 81​ helper = self.directives['python'].PythonDirective()
79 task = imp.load_source('runtask', '%s/%s' % (basedir, pyfilename)) 82​
83​ # prepare input for running the python directive
84​ input_args = {'callable': 'run', 'workingdir': self.workdir}
85​ env_args = {'basedir': basedir}
80 86​
81 # for now, assume that the only arg going into the script is workdir 87​ exec_output = helper.process(pyfilename, input_args, env_args)
82 exec_output = task.run(self.workdir) ​
83 88​
ralphUnsubmitted
Not Done

This line seems excessively long. Perhaps break it out for readability?

options = {'basedir': basedir, 'method_name': 'run', 'workingdir': self.workdir}
exec_output = helper.process(pyfilename, options, {})
tflinkAuthorUnsubmitted
Not Done

Actually, the entirety of this logic is going away soon (I've already started that refactoring) as it's way too hard-coded but that's no excuse for poor quality code :)

84 self.working_data.update({'exec_output':exec_output}) 89​ self.working_data.update({'exec_output':exec_output})
85 90​
86 def do_post(self, commands): 91​ def do_post(self, commands):
87 print "doing post with commands: %s" % str(commands) 92​ print "doing post with commands: %s" % str(commands)
88 93​
89def process_input(input_spec, args): 94​def process_input(input_spec, args):
90 print "verifying input: %s" % str(input_spec) 95​ print "verifying input: %s" % str(input_spec)
91 96​
Show All 39 Lines
View Options

pytest.ini

1[pytest] 1​[pytest]
2minversion = 2.0 2​minversion = 2.0
3python_functions=test should 3​python_functions=test should
4python_files=test_* testfunc_* 4​python_files=test_* functest_*
View Options

testing/conftest.py

Show All 23 Lines
24 24​
25 25​
26def pytest_addoption(parser): 26​def pytest_addoption(parser):
27 """ 27​ """
28 Add an option to the py.test parser to detect when the functional tests 28​ Add an option to the py.test parser to detect when the functional tests
29 should be detected and run 29​ should be detected and run
30 """ 30​ """
31 31​
32 parser.addoption('--functional', action='store_true', default=True, 32​ parser.addoption('--functional', action='store_true', default=False,
33 help='Add functional tests') 33​ help='Add functional tests')
34 34​
35 35​
36def pytest_ignore_collect(path, config): 36​def pytest_ignore_collect(path, config):
37 """Prevents collection of any files named testfunc* to speed up non 37​ """Prevents collection of any files named functest* to speed up non
38 integration tests""" 38​ integration tests"""
39 if path.fnmatch('*testfunc*'): 39​ if path.fnmatch('*functest*'):
40 try: 40​ try:
41 is_functional = config.getvalue('functional') 41​ is_functional = config.getvalue('functional')
42 except KeyError: 42​ except KeyError:
43 return True 43​ return True
44 44​
45 return not is_functional 45​ return not is_functional
46 46​
47 47​
48def pytest_configure(config): 48​def pytest_configure(config):
49 """This is a bit of a hack to detect whether or not we are running inside 49​ """This is a bit of a hack to detect whether or not we are running inside
50 a test environment""" 50​ a test environment"""
51 import sys 51​ import sys
52 52​
53 sys._called_from_test = True 53​ sys._called_from_test = True
View Options

testing/functest_python_directive.py

  • This file was added.
1​import os
2​import copy
3​from libtaskotron.directives import python_directive
4​
5​
6​class TestTaskMethod(object):
7​ def setup_method(self, method):
8​ self.ref_pyfile = 'some_loadable_test.py'
9​ self.ref_env = {'basedir': os.path.dirname(os.path.abspath(__file__))}
10​ self.test_directive = python_directive.PythonDirective()
11​
12​ def testfunc_task_method(self):
13​ ref_input = {'callable': 'task_method'}
14​
15​ test_output = self.test_directive.process(self.ref_pyfile, copy.copy(ref_input), self.ref_env)
16​
17​ assert test_output == "I'm a task method!"
18​
19​ def testfunc_class_embedded_call(self):
20​ ref_input = {'callable': 'embedded_task_target'}
21​
22​ test_output = self.test_directive.process(self.ref_pyfile, copy.copy(ref_input), self.ref_env)
23​
24​ assert test_output == "I'm a __call__ method in a class!"
25​
26​ def testfunc_class_target(self):
27​ ref_input = {'callable': 'task_class_target'}
28​
29​ test_output = self.test_directive.process(self.ref_pyfile, copy.copy(ref_input), self.ref_env)
30​
31​ assert test_output == "I'm a task class method!"
View Options

testing/some_loadable_test.py

  • This file was added.
1​class TaskClass(object):
2​ def my_task(self):
3​ return "I'm a task class method!"
4​
5​_instantiated_class = TaskClass()
6​
7​task_class_target = _instantiated_class.my_task
8​
9​class EmbeddedCallClass(object):
10​ def __call__(self):
11​ return "I'm a __call__ method in a class!"
12​
13​embedded_task_target = EmbeddedCallClass()
14​
15​def task_method():
16​ return "I'm a task method!"
View Options

testing/test_python_directive.py

  • This file was added.
1​from libtaskotron.directives import python_directive
2​from dingus import Dingus
3​import os
4​import imp
5​import pytest
6​import copy
7​
8​
9​class TestCheckfile(object):
10​ def test_existing_file(self, monkeypatch):
11​ ref_filename = 'turbo_encabulator.py'
12​
13​ stub_ospath = Dingus(exists__returns=True)
14​
15​ test_directive = python_directive.PythonDirective()
16​ monkeypatch.setattr(os, 'path', stub_ospath)
17​
18​ test_directive.checkfile(ref_filename)
19​
20​ def test_not_existing_file(self, monkeypatch):
21​ ref_filename = 'turbo_encabulator.py'
22​
23​ stub_ospath = Dingus(exists__returns=False)
24​
25​ test_directive = python_directive.PythonDirective()
26​ monkeypatch.setattr(os, 'path', stub_ospath)
27​
28​ with pytest.raises(Exception):
29​ test_directive.checkfile(ref_filename)
30​
31​
32​class TestLoadPyfile(object):
33​ def test_task_importname(self, monkeypatch):
34​ ref_filename = '/real/magic/turbo_encabulator.py'
35​ ref_taskname = 'runtask_turbo_encabulator'
36​ stub_imp = Dingus()
37​
38​ test_directive = python_directive.PythonDirective()
39​ monkeypatch.setattr(imp, 'load_source', stub_imp)
40​
41​ test_name, test_task = test_directive.load_pyfile(ref_filename)
42​
43​ assert test_name == ref_taskname
44​
45​
46​class TestExecutePyfile(object):
47​
48​ def test_task_method_is_found(self, monkeypatch):
49​ ref_modulename = 'foo'
50​ ref_methodname = 'bar'
51​ ref_kwargs = {'baz': 'why', 'blah': 2}
52​
53​ stub_getattr = Dingus()
54​
55​ test_directive = python_directive.PythonDirective()
56​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
57​
58​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
59​
60​ assert (stub_getattr.calls()[0].name, stub_getattr.calls()[0].args) == ('()', (ref_modulename, ref_methodname))
61​
62​ def test_task_method_is_executed(self, monkeypatch):
63​ ref_modulename = 'foo'
64​ ref_methodname = 'bar'
65​ ref_kwargs = {'baz': 'why', 'blah': 2}
66​
67​ stub_getattr = Dingus()
68​
69​ test_directive = python_directive.PythonDirective()
70​ monkeypatch.setattr(test_directive, '_do_getattr', stub_getattr)
71​
72​ test_directive.execute(ref_modulename, ref_methodname, ref_kwargs)
73​
74​ assert (stub_getattr.calls()[1].name, stub_getattr.calls()[1].args, stub_getattr.calls()[1].kwargs) == ('()', (), (ref_kwargs))
75​
76​
77​class TestProcess(object):
78​
79​ def setup_method(self, method):
80​ self.ref_command = 'foo.py'
81​ self.ref_input = {'callable': 'bar', 'kwarg1': 'baz'}
82​ self.ref_env = {'basedir': '/path/to/foo/'}
83​
84​ self.stub_checkfile = Dingus('checkfile')
85​ self.stub_imported = Dingus('imported')
86​ self.stub_pyload = Dingus('pyload', dostuff__returns=('name', self.stub_imported))
87​ self.stub_execute = Dingus('execute')
88​
89​ def test_process_checks_file(self, monkeypatch):
90​
91​ test_directive = python_directive.PythonDirective()
92​ monkeypatch.setattr(test_directive, 'checkfile', self.stub_checkfile)
93​ monkeypatch.setattr(test_directive, 'load_pyfile', self.stub_pyload.dostuff)
94​ monkeypatch.setattr(test_directive, 'execute', self.stub_execute)
95​
96​ test_output = test_directive.process(self.ref_command, copy.copy(self.ref_input), self.ref_env)
97​
98​ checkfile_call = self.stub_checkfile.calls[0]
99​
100​ assert (checkfile_call.name, checkfile_call.args, checkfile_call.kwargs) == ('()', (os.path.join(self.ref_env['basedir'], self.ref_command),), {})
101​
102​ def test_process_loads_file(self, monkeypatch):
103​
104​ test_directive = python_directive.PythonDirective()
105​ monkeypatch.setattr(test_directive, 'checkfile', self.stub_checkfile)
106​ monkeypatch.setattr(test_directive, 'load_pyfile', self.stub_pyload.dostuff)
107​ monkeypatch.setattr(test_directive, 'execute', self.stub_execute)
108​
109​ test_output = test_directive.process(self.ref_command, copy.copy(self.ref_input), self.ref_env)
110​
111​ pyload_call = self.stub_pyload.calls[0]
112​
113​ assert (pyload_call.name, pyload_call.args, pyload_call.kwargs) == ('dostuff', (os.path.join(self.ref_env['basedir'], self.ref_command),), {})
114​
115​ def test_process_executes(self, monkeypatch):
116​
117​ ref_kwargs = {'kwarg1': 'baz'}
118​
119​ test_directive = python_directive.PythonDirective()
120​ monkeypatch.setattr(test_directive, 'checkfile', self.stub_checkfile)
121​ monkeypatch.setattr(test_directive, 'load_pyfile', self.stub_pyload.dostuff)
122​ monkeypatch.setattr(test_directive, 'execute', self.stub_execute)
123​
124​ test_output = test_directive.process(self.ref_command, copy.copy(self.ref_input), self.ref_env)
125​
126​ execute_call = self.stub_execute.calls[0]
127​
128​ assert (execute_call.name, execute_call.args, execute_call.kwargs) == ('()', (self.stub_imported, self.ref_input['callable'], ref_kwargs), {})
129​No newline at end of file