New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an execution strategy flag #5981

Merged
merged 6 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@dotordogh
Copy link
Contributor

dotordogh commented Jun 19, 2018

In order to compile JVM things remotely, we need a flag to enable remote execution. This PR is an intermediary step. This PR only has nailgun and subprocess flags, which correspond to the behavior when nailgun is enabled and disabled, respectively. A future PR will add an additional option to this flag (hermetic) that will indicate that the run should be done remotely.

@dotordogh dotordogh requested a review from illicitonion Jun 19, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks for putting this together :)


@classmethod
def register_options(cls, register):
super(NailgunTaskBase, cls).register_options(register)
register('--use-nailgun', type=bool, default=True,
help='Use nailgun to make repeated invocations of this task quicker.')
register('--execution-strategy', type=str, default='',

This comment has been minimized.

@illicitonion

illicitonion Jun 19, 2018

Contributor

register has a choices kwarg, so you can:

register('--execution-strategy', choices=['nailgun', 'subprocess'], ...)

which will also do the validation for you :)

This comment has been minimized.

@dotordogh

dotordogh Jun 19, 2018

Contributor

I didn't realize choices was an actual thing. I couldn't find docs on it. I'll fix it.

@@ -19,12 +19,19 @@

class NailgunTaskBase(JvmToolTaskMixin, TaskBase):
ID_PREFIX = 'ng'
# Possible execution strategies:
NAILGUN = 'nailgun'
SUBPROCESS = 'subprocess'

@classmethod
def register_options(cls, register):
super(NailgunTaskBase, cls).register_options(register)
register('--use-nailgun', type=bool, default=True,

This comment has been minimized.

@illicitonion

illicitonion Jun 19, 2018

Contributor

We should deprecate this option, make sure that at most one of them is set, and set the other based on its value

This comment has been minimized.

@dotordogh

dotordogh Jun 19, 2018

Contributor

I wasn't sure if we wanted to go the whole way by deprecating it, but it seems like we do. I will update it!

register('--use-nailgun', type=bool, default=True,
help='Use nailgun to make repeated invocations of this task quicker.')
help='Use nailgun to make repeated invocations of this task quicker.',
removal_version='1.9.0.dev0', removal_hint='Please use --execution-strategy instead.')

This comment has been minimized.

@illicitonion

illicitonion Jun 21, 2018

Contributor

I think 1.10.0 is our earliest deprecation target at the moment :(

help='Use nailgun to make repeated invocations of this task quicker.')
help='Use nailgun to make repeated invocations of this task quicker.',
removal_version='1.9.0.dev0', removal_hint='Please use --execution-strategy instead.')
register('--execution-strategy', default='nailgun', choices=['nailgun', 'subprocess'],

This comment has been minimized.

@illicitonion

illicitonion Jun 21, 2018

Contributor

nit: Can probably use cls.NAILGUN and cls.SUBPROCESS instead of string literals here

def set_execution_strategy(self):
# This will be more complex as we add more execution strategies are added
# Expected behavior: if execution-strategy is set, it will override use-nailgun
if self.get_options().execution_strategy:

This comment has been minimized.

@illicitonion

illicitonion Jun 21, 2018

Contributor

Because there's a default value for this flag, this branch will always be hit, and we'll never look at use_nailgun. I ran into exactly the same issue when I changed an arg like this :)

So I think what we need to do is:

  1. Remove the defaults for both flags
  2. Set a default (nailgun) in this function if both are None
  3. Otherwise, do this logic you have - note that we should be doing a elif self.get_options().use_nailgun is False rather than elif not self.get_options().use_nailgun because None is falsey

May be worth adding a test that --use-nailgun=False actually works, or that may be overkill :)

Dorothy Ordogh
@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks, looks great!

self._identity = '_'.join(id_tuple)
self._executor_workdir = os.path.join(self.context.options.for_global_scope().pants_workdir,
*id_tuple)


def set_execution_strategy(self):

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

Oh, one last thing! This should probably be execution_strategy or get_execution_strategy, not set_! :)

@benjyw

benjyw approved these changes Jun 22, 2018

# --use-nailgun is deprecated
register('--use-nailgun', type=bool,
help='Use nailgun to make repeated invocations of this task quicker.',
removal_version='1.10.0.dev0', removal_hint='Please use --execution-strategy instead.')

This comment has been minimized.

@benjyw

benjyw Jun 22, 2018

Contributor

So polite! :)

@dotordogh dotordogh merged commit fdd6281 into pantsbuild:master Jun 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dotordogh dotordogh deleted the dotordogh:dotordogh/update-nailgun-flags branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment