Closed AndrewC19 closed 2 years ago
This is really excellent work! I want take some time this weekend to look/think it over. The spirit of it seems absolutely correct, and I suspect we'll just take the change wholesale.
Thanks, that sounds good to me. Happy to make changes to improve the quality of the changes when I get the time.
For operators that use arguments, this tuple needs to store information about the arguments used for each test (currently this information is put in a comment next to the tuple). I couldn't find an elegant solution for this problem as the example method is a class method, so has no access to the instance variables.
It seems to me that the examples()
classmethods could simply include any necessary arguments in the returned tuple (as a side note, we should probably return something more structured from those functions, e.g. a dataclass). So the examples for VariableReplacer
might be:
@classmethod
def examples(cls):
return (
('y = x + z', 'y = 10 + z', 0, {'cause_variable': 'x'}),
('j = x + z\ny = x + z', 'j = x + z\ny = -2 + z', 0, {'cause_variable': 'x', 'effect_variable': 'y'}),
# etc...
)
Those could be **-unpacked into the constructor call in the test. Does make sense? Am I missing some angle?
I'm broadly happy with the direction of your changes. I think it needs a little work in some areas, we'll get operator arguments in there one way or the other. Take a look at the review notes I made and let me know if you have any questions. It's entirely possible that I've missed the point of some things you've done, so let me know if that's the case.
I also think the HTTP distributor needs to be considered in light of operator arguments. Are the arguments making it across the HTTP call, and are they being used? I know we don't have any tests for this, so it's easy to miss this.
I don't think so. If you can direct me to the HTTP call then I can implement the necessary changes.
I also think the HTTP distributor needs to be considered in light of operator arguments. Are the arguments making it across the HTTP call, and are they being used? I know we don't have any tests for this, so it's easy to miss this.
I don't think so. If you can direct me to the HTTP call then I can implement the necessary changes.
That's in cosmic_ray/distribution/http.py
For operators that use arguments, this tuple needs to store information about the arguments used for each test (currently this information is put in a comment next to the tuple). I couldn't find an elegant solution for this problem as the example method is a class method, so has no access to the instance variables.
It seems to me that the
examples()
classmethods could simply include any necessary arguments in the returned tuple (as a side note, we should probably return something more structured from those functions, e.g. a dataclass). So the examples forVariableReplacer
might be:@classmethod def examples(cls): return ( ('y = x + z', 'y = 10 + z', 0, {'cause_variable': 'x'}), ('j = x + z\ny = x + z', 'j = x + z\ny = -2 + z', 0, {'cause_variable': 'x', 'effect_variable': 'y'}), # etc... )
Those could be **-unpacked into the constructor call in the test. Does make sense? Am I missing some angle?
I have implemented an Example
dataclass which stores the pre and post mutation code (mandatory), occurrence index (optional), and mutation args (optional). This is a more consistent solution than relying on a tuple, in my opinion. All existing operators' examples have been updated to use this structure and their tests pass.
On this topic, I have encountered an issue with the operator tests for VariableRemover
and VariableInserter
: both randomly sample a value for the mutation (e.g. a random numerical value) and therefore I cannot say the precise value that a given mutation will have (since it is random). One solution would be to set a seed for the tests; what are your thoughts?
One solution would be to set a seed for the tests; what are your thoughts?
This is an interesting topic I haven't thought about much. I'm not sure about the desirability of randomness in mutation operators, but I guess CR has to accept the possibility since it can be extended. Fixing the seed is probably a good first attempt...it should at least allow us to get the tests passing. Another option is that we allow Examples to specify a custom 'comparison' function when simple string comparison won't be enough. I need to think about this.
I think what I'm going to do is merge this in as-is, and then I'll make some changes I want. My biggest concern at this point is that providing arguments for any operator essentially turns off the other non-argument-having operators. This seems incorrect to me. If we're going to have general support for arguments, I see no good reason to make such a big mode switch when any arguments are supplied; providing arguments and disabling operators seem like orthogonal concerns to me.
So I'm going to merge this in and make some changes afterward. If you want to be able to run only a subset of operators, I think we have existing features for this. If not, you can write a simple filter to do this.
Great, that sounds good to me.
There are a few issues that still need fixing from this PR:
Hope that helps!
Overview
This PR enables users to define mutation operators with arbitrary key word arguments. As an example, the
VariableReplacer
andVariableInserter
operators have been added. These take two arguments, acause_variable: str
and aneffect_variable: str
, and enable the following three types of mutation:(1) Replacing all usages of a specific variable with a random numerical constant (e.g.
VariableReplacer('x1')
mutatesy=x1+x2+x3+10
toy=30+x2+x3+10
). (2) Replacing usages of a specific variable that appear in the statement of another specific variable (e.g.VariableReplacer(x1, y)
mutatesy=x1+x2+x3+10
toy=30+x2+x3+10
but does not mutatej=x1+x2+x3+10
). (3) Adding usages of a specific variable in the statement of another specific variable (e.g.VariableInserter(x, y)
mutatesy=2*z+1
toy=2*z+1*x
).To specify the variables of interest, the user can now add the following table to their TOML config file:
If such a table is added, only the specified mutation operators will be applied (notice that args are only supplied to operators with args). Otherwise, as before, all mutation operators are tried during initialisation. This will not apply any operators that require arguments.
Changes
As part of this PR, the following changes have been made:
(
src/cosmic_ray/operators/variable_replacer.py
): A new operator that replaces named variable usages, as described above.(
src/cosmic_ray/operators/variable_inserter.py
): A new operator that added named variable usages, as described above.(
src/cosmic_ray/operators/provider.py
):provider.py
now includesVariableReplacer
in the available operators.(
src/cosmic_ray/work_item.py
):ResolvedMutationSpec
now includes a dictionary of operator arguments as an additional optional argument.(
src/cosmic_ray/work_db.py
):work_db.py
now stores an additional column in theMutationSpecStorage
table for the operator arguments. These are stored as a JSON object, enabling an arbitrary number of key word arguments to be stored and retrieved.(
src/cosmic_ray/config.py
):config.py
now has an additional propertyoperators
that stores the specified operators and their args.(
src/cosmic_ray/cli.py
): If thecosmic-ray.operators
table is present in the config file, it is extracted and passed as a dictionary tocosmic-ray.commands.init
. This dictionary stores thename
andargs
(optional) of the operator.(
src/cosmic_ray/commands/init.py
):init.py
now instantiates a dictionary storing all operator names (as before) only if the operator config hasn't been provided. Otherwise, it passes the dictionary containing the specified operators and args to_all_work_items
. Then,_all_work_items
handles this dictionary by instatiating an operator with any specified args and identifiying mutation positions, as before.(
src/cosmic_ray/mutating.py
):mutating.py
now passes any arguments to the instantiation of the operator by unpacking themutation_args
dict. This is an empty dict if no args have been specified. A try except is used to handle cases where no args are given (e.g. as intest_no_test_return_value
).TODOs:
The following problems still need addressing before merging the PR:
HTML Report: Currently, the HTML report does not output information about the arguments of the
VariableReplacer
operator. It would be good if the report listed the arguments of the mutation operator if there are any.Tests based on
Operator.example
class method: Currently, thetest_operators.py
tests use the tuple defined in theexample
method to build an assertion. For operators that use arguments, this tuple needs to store information about the arguments used for each test (currently this information is put in a comment next to the tuple). I couldn't find an elegant solution for this problem as theexample
method is a class method, so has no access to the instance variables.Update
example
method ofVariableInserter
. This currently has the body ofVariableReplacer
'sexample
method.