[LNT] Python 3 support: remove useless var-setting getter

[thopre]

Summary

Both the ComparisonResult constructor and the stddev_mean getter set
the readonly stddev_mean. While Python 2 accepts it, Python 3 throws
errors (cannot set attribute and infinite recursion respectively). Since
that attribute does not appear to be used anywhere, this commit removes
all this code.

[thopre]

Ping?

[hubert.reinterpretcast]

Perhaps others are more well-versed in this than I am, but I think that having a link in the commit message and using the terminology used by the documentation ("new-style" and "classic") would be useful here: https://docs.python.org/2/reference/datamodel.html#newstyle. Also, I am not sure that switching these to be new-style classes in Python 2 is necessary. I believe the commit message should give additional rationale, e.g., using new-style classes helps make the Python 2 and Python 3 behaviour of the code more similar.

lnt/server/reporting/analysis.py

109

This seems to work fine if the name of the caching variable and the name of the property is not the same.

114

This makes it possible to assign to the property, which was read-only before this change.

[thopre]

In D67882#1697371, @hubert.reinterpretcast wrote:

Perhaps others are more well-versed in this than I am, but I think that having a link in the commit message and using the terminology used by the documentation ("new-style" and "classic") would be useful here: https://docs.python.org/2/reference/datamodel.html#newstyle. Also, I am not sure that switching these to be new-style classes in Python 2 is necessary. I believe the commit message should give additional rationale, e.g., using new-style classes helps make the Python 2 and Python 3 behaviour of the code more similar.

Good point about terminology. I'm not sure I understand your point about it not being necessary in Python 2. All the changes I'm doing in this Python 3 support series are not necessary for Python 2, they are here to have a codebase that works accross both versions of Python.

In D67882#1697371, @hubert.reinterpretcast wrote:

Perhaps others are more well-versed in this than I am, but I think that having a link in the commit message and using the terminology used by the documentation ("new-style" and "classic") would be useful here: https://docs.python.org/2/reference/datamodel.html#newstyle. Also, I am not sure that switching these to be new-style classes in Python 2 is necessary. I believe the commit message should give additional rationale, e.g., using new-style classes helps make the Python 2 and Python 3 behaviour of the code more similar.

I hadn't realized that the explicit inheritance from object was not a requirement of Python 3. Therefore as you say it's only to improve behaviour being more similar between Python 2 and 3 but given the timeframe and the low amount of commits on LNT I don't think that is necessary. I think we should make LNT Python 3 only as soon as it can work in that mode, so the less compability code we can add the better. Do you agree with that approach?

[thopre]

Remove class style compability change
Remove stddev_mean property code altogether since it is not used anywhere

[hubert.reinterpretcast]

In D67882#1697616, @thopre wrote:

I think we should make LNT Python 3 only as soon as it can work in that mode, so the less compability code we can add the better. Do you agree with that approach?

I agree with the approach of minimizing compatibility code. I am not sure about proactively dropping Python 2 support. As you said, the amount of commits going into LNT is low, so keeping Python 2 compatibility once the Python 3 mode works won't cost much development effort.

[thopre]

In D67882#1697713, @hubert.reinterpretcast wrote:

In D67882#1697616, @thopre wrote:

I think we should make LNT Python 3 only as soon as it can work in that mode, so the less compability code we can add the better. Do you agree with that approach?

I agree with the approach of minimizing compatibility code. I am not sure about proactively dropping Python 2 support. As you said, the amount of commits going into LNT is low, so keeping Python 2 compatibility once the Python 3 mode works won't cost much development effort.

It's not so much the development effort rather than the risk of introducing python2- or python3-specific code without noticing. Since LNT is an application so has no problem of reverse dependences and Python 2 becomes EOL in 2 months I feel like a migration allows to quickly remove the compatibility added without much downside. But either way it doesn't impact the amount of effort to migrate so I'm fine with whatever is decided.

[hubert.reinterpretcast]

LGTM.

lnt/server/reporting/analysis.py

def __init__(self, aggregation_fn,
         # functions in this case.
         if samples and len(samples) > 1 and isinstance(samples[0], float):
             self.stddev = stats.standard_deviation(samples)
             self.MAD = stats.median_absolute_deviation(samples)
         else:
             self.stddev = None
             self.MAD = None
 
-        self.stddev_mean = None  # Only calculate this if needed.
         self.failed = cur_failed
         self.prev_failed = prev_failed
         self.samples = samples
         self.prev_samples = prev_samples
 
         self.confidence_lv = confidence_lv
         self.bigger_is_better = bigger_is_better
 
-    @property
-    def stddev_mean(self):
-        """The mean around stddev for current sampples. Cached after first call.
-        """
-        if not self.stddev_mean:
-            self.stddev_mean = stats.mean(self.samples)
-        return self.stddev_mean
-
     def __repr__(self):
         """Print this ComparisonResult's constructor.
 
         Handy for generating test cases for comparisons doing odd things."""
         fmt = "{}(" + "{}, " * 9 + ")"
         return fmt.format(self.__class__.__name__,
                           self.aggregation_fn.__name__,
                           self.failed,
                           self.prev_failed,
                           self.cur_hash,
                           self.prev_hash,
                           self.samples,
                           self.prev_samples,
                           self.confidence_lv,
                           bool(self.bigger_is_better))