An implementation of Subsystems.

Review Request #2063 — Created April 11, 2015 and submitted

benjyw
pants
e9507d6...
pants-reviews
ity, jsirois, patricklaw, zundel
- Provides a Subsystem base class.
- Provides all the machinery for declaring subsystem use,
  subsystem option registration etc.

To keep this change reasonably-sized, no actual subsystems are provided.
However I have battle-tested this code by retrofitting two existing
classes (RunTracker and JarTool) to be subsystems. That code will
be submitted in a separate change, for ease of review.

CI passes, apart from presumably-unrelated jar shading issues:
https://travis-ci.org/pantsbuild/pants/builds/58083895
(the failing test passes when I run it locally)

JS
  1. This _should_ address the spurious shading issues under TravisCI: https://rbcommons.com/s/twitter/r/2065/
  2. 
      
JS
  1. 
      
  2. src/python/pants/subsystem/BUILD (Diff revision 1)
     
     
    Dedent here down 2, I did a double-take on the opening ( not being matched even though it actually is matched.
    1. Fixed. My IntelliJ setup does something weird with BUILD files.

  3. src/python/pants/subsystem/subsystem.py (Diff revision 1)
     
     

    How about just separate registration from instances - pass a Subsystems registry instance into the Task callbacks. They can call subsystems.register(subsytem_type, *args, **kwargs) (allow them to curry extra constructor args). Then the Subsytem abstract class can be completely seprated from the Subsystems registry and there is no mutable class state in sight.

    1. I'm not sure I understand what you're proposing? This isn't directly related to registration but to how subsystem instances get hold of their parsed option values, even though we create subsystem instances lazily (unlike tasks).

  4. 
      
ST
  1. 
      
  2. src/python/pants/option/options.py (Diff revision 1)
     
     
     
     
     
     
     

    Making this an actual object would allow for real docstrings. But what is the usecase for accessing bootstrap options?

    1. Some options want to set a default value based on the value of bootstrap options, so we make them available inside register().

      Regarding your first point, if I were a functional purist I would say "a function is an actual object, how dare you!" But in truth yes, it's a bit weird as it stands, tacking properties onto a function, and I have a mental TODO to convert it to a class instance. I'll add a real TODO in the code...

  3. src/python/pants/subsystem/subsystem.py (Diff revision 1)
     
     

    Is a Task technically a Subsystem? Would it complicate or clarify things to give them a shared mixin for registration?

    1. Very good question. I'm not yet sure if a Task is a Subsystem, or if the entire task-running mechanism in GoalRunner is, or neither. I started out with a shared mixin, but it made the scope of this change bigger than I wanted, so I abandoned it Let's revisit later?

  4. 
      
BE
ZU
  1. Thank you for taking this on, I think this bit will make the rest of options handling fall neatly into place!

  2. src/python/pants/subsystem/subsystem.py (Diff revision 2)
     
     

    I'm trying to understand this better. In goal_runner.py, there is a notion that the subsystem itself is global, but it seems like it is a bit more nuianced than that. By having different extension points, it seems like in the same subsystem you could have both global and task specific options. Or is this the same options registered at different scopes? I think it is the latter, but seeing as this is a base class for users to extend, its a bit confusing.

    Is adding lots of logic in here for handing global vs tasks specific registration handled inside of this class the right thing to do? I can't think of why a subclass would want to override these methods. Could register_options_for_global_instance, register_options_for_task_instance and _register_options_on_scope belong outside of this object in some kind of overall options registration logic?

    I have the same comment around the methods global_instance() and instance_for_task() below. It seems like the author of a subsystem really doesn't need to concern themselves with task vs. global scope - the options registration logic can just take care of making sure the options are registered in both places and this class could not expose all that yuckiness.

    1. The author of a subsystem indeed doesn't need to know about scopes, she just implements register_options(), like for tasks. The code has no idea if it's running in a global or task-specific instance. The options registration logic (in goal_runner.py and in task.py) does indeed take care of doing the right thing: it calls the subsystem's register_options() once per global instance and once per task instance (if any).

      These extra methods are just for convenience, and indeed are not intended to be overridden (as the docstring says), although I thought there might be cases where we'd want a subsystem to be able to distinguish between global and per-task registration. However I can remove them and put the logic directly in goal_runner.py / task.py if you think they're confusing. If we ever need that distinction (and I hope we never do) then we can figure something out...

  3. 
      
BE
ZU
  1. 
      
  2. src/python/pants/subsystem/subsystem.py (Diff revision 3)
     
     

    nit: This and scope_qualifier() are the only methods to be implemented by subclasses, right? swap position with register_options_on_scope to keep them up at the top.

  3. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 690e1873a3f09a350cdfa10d8b9a6a4cf8011dc9.

Loading...