Defer argparse registration to the last possible moment.

Review Request #3049 - Created Oct. 28, 2015 and submitted

Information
Benjy Weinberger
pants
d426b62...
Reviewers
pants-reviews
zundel
Now option registration just records the registration args,
and they get applied as needed, as an implementation detail.

This was also an opportunity to clean up and de-complicate
how we register recursive options, so the code is neater now.

The performance benefits are very noticeable in profiles.

Note that this means that some validation will only happen if
the options are actually requested, which required a few
changes in tests.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/88011835

Issues

  • 0
  • 4
  • 0
  • 4
Description From Last Updated
Eric Ayers
Stu Hood
Stu Hood
Benjy Weinberger
Benjy Weinberger
Benjy Weinberger
Review request changed

Status: Closed (submitted)

Change Summary:

65cd3bc9338aeb7efe02c13eea9e7b9def3435b9

Nick Howard (Twitter)

   
src/python/pants/option/parser.py (Diff revision 1)
 
 
 

It seems like this will reregister options for every explicit scope in a command.

For each explicit scope, we'll relook up the default values for all flags set in that scope, as well as re normalizing.

Could we memoize the registration rather that recomputing the list of args and calling _argparse_register?

Could we also lazily instantiate the argparser itself?

  1. I had a change pending anyway to lazily instantiate the argparser. Will add the other fixes to that one.

    I'm not sure what you mean by "every explicit scope in a command"? We definitely aren't re-registering the same options on the same ArgParser, it chokes if you try that.

  2. What I mean is that for every usage of a scope in a cli invocation, ie compile.scala --foo, or --compile-scala-foo, this code will be executed. If we created and initialized the argparser if not already done, it wouldn't be an issue.

    We don't re-register against the argparser instance because of the guard in _argparse_register, but we will call _argparse_register for already registered scopes if they are referenced multiple times on the command line.

  3. I still don't see what you're getting at? This is called (at most) once per scope, in the invocation of options.for_scope(). The cli invocation should have no effect at all on the control flow here.

  4. I see what you mean. I got a bit turned around. Sorry.

src/python/pants/option/parser.py (Diff revision 1)
 
 

It's interesting that this is parent first, but recursive_option_registration_args is current, then parent.

I think it should be ordered parent first the whole way, rather than having the parents of the current scope first, but reversed. Is this ordering tested somewhere?

  1. It doesn't matter for correctness, but I agree that it would be neater to reverse the order in _recursive_option_registration_args(), so I'll add that to my pending change.

    The ordering isn't tested because it doesn't matter.

  2. I would disagree about ordering not mattering, as it's used by the help printers which is part of pants' user interface. But you're right that it shouldn't affect correctness.

  3. The help printers do their own ordering, which is as it should be. This class has its own ordering needs...

  4. Right. I think I understand now.

src/python/pants/option/parser.py (Diff revision 1)
 
 

This seems like a new validation. If there isn't a test covering it, could you add one?

  1. No, that was in there before (with a somewhat different error message phrasing, perhaps). I can add a test for it though.

  2. Ok. I didn't see it in the diff. Thanks.

src/python/pants/option/parser.py (Diff revision 1)
 
 

Should this check all the args? Registrations can register multiple flags. eg '--changes-since', '--parent', in what_changed.py

  1. Yes, it should. I had considered the case of the parent registering multiple flags, but not the shadower. Fixed, and added a test.

Loading...