Skip to content
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

use `type=` to register enum options #7304

Merged

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented Mar 2, 2019

Problem

Resolves #7233. We want to make it easy and natural to register enum options, and one way to improve that is to automatically convert options registered as enums to instances of those enums.

Solution

  • Convert hyphenated enum values to underscored classpropertys, e.g. 'zinc-only' becomes .zinc_only.
  • Remove register_enum_option() and use type= to automatically convert option values to enum instances.
  • Make __new__ for enum classes accept an instance of the enum class and pass it through so options parsing can assume the constructor is idempotent.
  • Wrap the type checking area in options parsing in a try and convert to a ParseError with more information about the invalid option.
  • Extract .all_variants from a type arg and use it for choices if not explicitly specified, allowing enum type options to automatically populate the choices argument to register().
  • Extract datatype and enum logic into DatatypeMixin (to do special-case logic in stable_json_sha1()) and ChoicesMixin (to do the special-case logic to get choices).

Result

Registering options as enum classes is much more natural, and has a better error message!

@Eric-Arellano
Copy link
Contributor

left a comment

Cool!

Question, with uses of register_option can you set the kwarg type=Enum? That or a variation would be variable helpful to add if it's possible. Right now it seems to be a bit implicit when an option is an enum.

Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step.py Outdated
Show resolved Hide resolved src/python/pants/option/global_options.py Outdated
Show resolved Hide resolved src/python/pants/option/parser.py
Show resolved Hide resolved src/python/pants/option/parser.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved src/python/pants/option/parser.py Outdated
Show resolved Hide resolved src/python/pants/option/parser.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:allow-enum-option-type branch 2 times, most recently from 5341fd2 to 823af03 Mar 2, 2019

@cosmicexplorer cosmicexplorer changed the title add a .register_option() method to enum and use `type=` to register enum options use `type=` to register enum options Mar 3, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

Question, with uses of register_option can you set the kwarg type=Enum? That or a variation would be variable helpful to add if it's possible. Right now it seems to be a bit implicit when an option is an enum.

Changed to instead explicitly just register(..., type=MyEnumType, ...), and to use choices=MyEnumType.iterate_enum_variants() for types which have that attribute!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:allow-enum-option-type branch 2 times, most recently from 5a9e073 to 3363df0 Mar 3, 2019

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Thanks for the changes - I like the way this looks now! Just a couple more comments.

@benjyw

benjyw approved these changes Mar 4, 2019

Copy link
Contributor

left a comment

Good stuff!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:allow-enum-option-type branch 3 times, most recently from 0115b1c to a2a9700 Mar 4, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

There were multiple changes I made just now to pass testing (or at least all the errors I saw in previous CI runs):

  • Add another elif in CoercingEncoder#default() used in stable_json_sha1() to process datatypes (and enums are datatypes) since otherwise it tries to iterate over them when trying to fingerprint options (and datatype's __iter__ intentionally raises).
  • Use enum instances as default= kwargs in all cases instead of raw strings, otherwise testing will fail because e.g. self.get_options().execution_strategy is a string -- I believe this may be because tests won't always call the type argument due to the use of fake options, but I'm not sure.
  • Use ensure_bytes() when writing to a file descriptor in nailgun_client.py. I have no clue why this became necessary and suspect it is because we are using a nailgun where we weren't before as a result of changing the --execution-strategy to being an enum, and I have no clue whether this is more or less correct. Alternatively, this may just repro locally, and I happen to have a different encoding set than in CI. Not sure whether this should be a concern at all.

I might ask for a re-review once this goes green (I did not realize these changes would be required to do this) -- but I don't expect there to be further surprises as everything I've tried passes locally.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:allow-enum-option-type branch 3 times, most recently from 84906be to 77c6de8 Mar 4, 2019

@illicitonion
Copy link
Contributor

left a comment

Yay!

@ity

ity approved these changes Mar 5, 2019

Copy link
Contributor

left a comment

looks great!

options = self._parse('./pants enum-opt --some-enum=invalid-value')
options.for_scope('enum-opt').some_enum
self.assertIn("""\
ParseError: Error applying type 'SomeEnumOption' to option value 'invalid-value', for option '--some_enum' in scope 'enum-opt': type check error in class SomeEnumOption: Value {u}'invalid-value' must be one of: [{u}'a-value', {u}'another-value']."""

This comment has been minimized.

Copy link
@ity

ity Mar 5, 2019

Contributor

this is quite specific, maybe shave it down to the just the errors args such as invalid-value etc. any slight change to the exception message will make this fail.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Author Contributor

Fixed! This was proven correct as an incorrect error message here was causing the CI failure.

This comment has been minimized.

Copy link
@ity

ity Mar 6, 2019

Contributor

looks good!

cosmicexplorer added some commits Mar 2, 2019

cosmicexplorer added some commits Mar 2, 2019

remove .register_option() enum classmethod and idempotent type arg, a…
…nd extract choices from enum option types

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:allow-enum-option-type branch from 831ae26 to 6bd7855 Mar 6, 2019

@@ -348,8 +347,8 @@ def get_some_union_type(x):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Author Contributor

This started erroring out here when running locally and in CI without these (necessary) changes. I'm not sure why this is the case, but that is why this file is changed in this PR.

@@ -22,6 +21,7 @@
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import safe_mkdir, safe_open
from pants.util.memo import memoized_property
from pants.util.strutil import ensure_text
from pants.version import PANTS_SEMVER

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Author Contributor

This was breaking with newstr does not support decode, which kind of makes sense.

@@ -17,6 +17,7 @@
from pants.java.nailgun_protocol import ChunkType, NailgunProtocol
from pants.util.osutil import safe_kill
from pants.util.socket import RecvBufferedSocket
from pants.util.strutil import ensure_binary


This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Author Contributor

This was failing when running with ./pants2, and I'm not sure why, but it seems like a correct edit to make regardless.

@@ -20,12 +21,10 @@ def __call__(cls, *args, **kwargs):
class ClassPropertyDescriptor(object):
"""Define a readable attribute on a class, given a function."""

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 6, 2019

Author Contributor

This just removes the mention of @memoized_classproperty, which exists now.

@ity

ity approved these changes Mar 6, 2019

@cosmicexplorer cosmicexplorer merged commit ef04365 into pantsbuild:master Mar 6, 2019

1 check passed

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

@cosmicexplorer cosmicexplorer referenced this pull request Apr 15, 2019

Open

extract options parsing into its own library #7567

0 of 2 tasks complete

ity added a commit to twitter/pants that referenced this pull request Apr 30, 2019

ity added a commit that referenced this pull request Apr 30, 2019

stuhood added a commit that referenced this pull request May 7, 2019

Short-circuit primitive encoding on CoercingEncoder (#7655)
### Problem

`CoercingEncoder.encode()` is called many times when fingerprinting options. There is a significant overhead to the `isinstance()` calls in python [reference](https://stackoverflow.com/questions/42378726/why-is-checking-isinstancesomething-mapping-so-slow). This means that, when `datatype`s recursively encode themselves after #7304, they do twice the checks that they already did, causing a several-second regression in large targets (from 56s to 59s, without pantsd).

### Solution

Short-circuit primitives to avoid some of those checks.

There is probably more reordering of branches to be done, but I don't know enough about the callers of that function to reorder safely, since the if-else branches return early and thus the cases might not be a real partition of the set of possible inputs.

### Result

The noop compile times of that large target are back to 56s.

### Acknowledgements

@ity and @stuhood for finding the regressing commit, and @cosmicexplorer and @illicitonion for helping find the root cause.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.