Add RecursiveVersion and tests

Review Request #3331 - Created Jan. 13, 2016 and submitted

Information
Matt Olsen
pants
2531, 2795
Reviewers
pants-reviews
kwlzn, nhoward_tw, stuhood

Add version to task and add verion to fingerprint

In order to make sure that we can invalidate cached objects based on the implementation version of a task we allow the option to specify a recursive version. Our cached directories and fingerprints then use the version to make sure we do not have a cache hit on incompatible versions. In order to bump the version of a Task object you would need to bump the class attribute version for example:

class Foo(TaskBase):
version = RecursiveVersion(1)

would become

class Foo(TaskBase):
version = RecursiveVersion(2)

green on my personal fork

CI is green https://travis-ci.org/pantsbuild/pants/builds/101983901
CI is green https://travis-ci.org/pantsbuild/pants/builds/102197460
CI is green https://travis-ci.org/pantsbuild/pants/builds/102225231
CI is green https://travis-ci.org/pantsbuild/pants/builds/102450529

Issues

  • 5
  • 17
  • 2
  • 24
Description From Last Updated
cls is idiomatic. Patrick Lawson Patrick Lawson
Yikes. This is very magical. Why is this an attribute with a lot of meta-programming rather than a classmethod that ... Patrick Lawson Patrick Lawson
parents and parent_versions and the inline class_version function should all just be a single list comprehension. Patrick Lawson Patrick Lawson
Scary. Is version really that privileged of a class attribute name? I could imagine it referring to something unrelated to ... Patrick Lawson Patrick Lawson
In general, prefer list/generator comprehensions to python's "functional" programming constructs. Patrick Lawson Patrick Lawson
Matt Olsen
Nick Howard (Twitter)
Matt Olsen
Nick Howard (Twitter)
Matt Olsen
Matt Olsen
Nick Howard (Twitter)
Matt Olsen
Kris Wilson
Matt Olsen
Eric Ayers
Matt Olsen
Review request changed

Status: Closed (submitted)

Change Summary:

commit b1c0b1b

Patrick Lawson

   

cls is idiomatic.

  1. Generally I've seen cls used in @classmethods however when you are poking at the actual class object usually I've seen klass or class_. If this is confusing I'm fine with changing it though.

Yikes. This is very magical. Why is this an attribute with a lot of meta-programming rather than a classmethod that calls super? The latter gets you the same behavior (it walks up the MRO, without explicitly invoking the MRO reflective code), and it also skips all classes in the hierarchy that don't wish to be rolled into the version.

Note that right now adding or reordering the mixin inheritance chain invalidates all of the fingerprints, even if the mixins nominally don't contribute to the versioning.

  1. This blog does a pretty good job of explaining why using super for this problem is a big headache: http://yergler.net/blog/2011/07/04/super-self/

  2. I believe Patrick means that Task.version should be a classmethod that calls super(MyTaskName, self).version() rather than using this external function that does reflection on the Task hierarchy (and which indeed cannot use super). And that does seem a lot simpler to me too.

  3. I don't see why it's an issue. In a Task subclass MyTask you would define

    @classmethod
    def version(cls):
      return super(MyTask, cls).version().add_version(3)
    

    The super-class hierarchy is well defined, there is no magic. All you need is a default implementation on the Task class and some trivial class that version() returns which implements add_version and get_version in the obvious ways.

  4. I spoke with Patrick via slack about the implementation. We've decided that using an explicit call to super similar to the way options is handled will be clearer. The implication is that all tasks will have to define a @classmethod and the api changes slightly. I am reverting this change so that no one uses this method to specify version while I update the code.

parents and parent_versions and the inline class_version function should all just be a single list comprehension.

Scary. Is version really that privileged of a class attribute name? I could imagine it referring to something unrelated to this metaprogramming.

In order to do this metaprogramming safely, you really end up needing a proper metaclass that rewrites the object dictionary with an obfuscated name or maintains a dictionary on the object which has an obfuscated attribute name. This is how lots of ORMs in python work.

But that kind of metaprogramming is mega-overkill, and should be avoided unless it provides significant value. Here I think we should stick with classmethods and do away with all of the magic, as mentioned above.

  1. I'm not sure what you mean by version being priviledged. Would you mind providing an exmple of the type of problem you are trying to avoid?

    The reason I'm reacing into .dict is to avoid recursing into the parents version and hitting the parent descriptor class. This code is using the descriptor protocol, which is similar to ORM's like django handle table definitions. https://docs.python.org/2/howto/descriptor.html

  2. What if I have some mixin in the Task hierarchy that looks like:

    class MyMixin(object):
      version = SomeOpaqueThing()
    

    Your code will see this and try to roll it into the version, even though that's not the intent.

    Once again, I'm strongly opposed to duplicating ORM metaprogramming patterns without careful consideration as a group. It invariably leads to maintenance pain in the long run. It's better to be explicit than to rely on this kind of magic, especially when there's a reasonable alternative (regular classmethod) and it's not being used in a class dominated by its DSL nature. Tasks are not a DSL, and they already have a fairly complex lifecycle. Tossing metaprogramming into the mix is dangerous and unnecessary.

In general, prefer list/generator comprehensions to python's "functional" programming constructs.

Patrick Lawson

And while I'm in here, I'd like to call out the fact that this relatively important change did not have any cross-org reviewers. I know the recent mention of this policy happened after this change was submitted, but it's a good example of the type of commit that should have a more diverse set of eyes on it.

Loading...