-
-
Notifications
You must be signed in to change notification settings - Fork 636
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 "named interpreter constraints" #12652
Comments
I like this! Why isn't the option dict-valued? |
Because:
[python-setup]
# Awkward string escaping
interpreter_constraints = "{'default': ['>=3.6,<3.10'], 'py2': ['==2.7.*']}" [python-setup]
# Note how clunky it is to set a single default for the whole repo
interpreter_constraints = "{'default': ['>=3.6,<3.10']}" It's not possible for this option to be sometimes a list (if you just have one global default) vs. sometimes a dict. Wdyt? |
That doesn't seem particularly clunky to me. It's just a normal Python dict literal. And avoiding dict-valued options because we haven't provided good support for them in pants.toml seems like a big smell... |
Probably we can start supporting toml nested tables, now that option subscopes are no longer a thing? |
That would be pretty sweet. [python-setup]
interpreter_constraints = { default = [">3.6,<4"], py2 = ["==2.7.*"] } aka [python-setup.interpreter_constraints]
default = [">3.6,<4"]
py2 = ["==2.7.*"] Much much more natural. And then if you want to use the CLI or env vars, you'd have to use the Python syntax. Same with the add/remove syntax. Note that "named resolves" will be implemented this same way, so getting this right will be useful. -- If we have to add back subscopes, we can redesign a more TOML-tolerant mechanism like using |
I can take a look at the toml thing. Unless, Eric, you already know how to do it and it's easy. |
It's not super complex fwict and I wrote the original TOML parser, so I'd be happy to do it. But feel free if it sounds fun and you're looking for things to do! I wouldn't be able to get to it till next week with tool lockfiles being stable. Before embarking on it though, are we confident we won't add back subscopes that use |
Subscopes were very complicated for both Pants developers and users, and I would be reluctant to add them back, especially as a general name-based mechanism. But yeah, if we do, we can use something other than |
FYI a subscope still exists in 2.7: |
Yeah, good point. Let's wait until we can do that one deprecation. |
Native TOML dictionaries are now supported! Based on developments with named resolves, I'm now thinking this modeling for named interpreter constraints: [python-setup]
possible_interpreter_constraints = { py2 = "==2.7.*", py3 = ">=3.6,<4" }
default_interpreter_constraints = "py3" python_library(interpreter_constraints="py2") (Or See #12742 for the two competing proposals for how to model the default ICs. From there:
This is more boilerplate when you only have a single interpreter constraint in your repository than the status quo. That's a bummer, but I think it's minimal enough and worth it. Reminder that you only have to set these two options once usually, and usually done by a codebase admin. |
@Eric-Arellano : Two completely wildcard ideas which have have been floating around in my mind (and which I'm very uncertain about).
|
I remain very concerned about the performance hit of calculating interpreter constraints in a single-IC repo, and our users have pointed it out, prompting changes like #15141. I'm not sure how I feel about named interpreter constraints still. My concern is you have to create this custom naming mechanism and have all your internal users know what names to use. Consider this:
Is that confusing to users to discover those names? The major benefit I see still is that admins have tight blessing over which ICs are valid in a repo. That's desirable. -- Reminder that if we don't use named interpreter constraints, we can do something like #12495 to fix performance. Thoughts @thejcannon @stuhood @benjyw ? |
For the purposes of resolving the performance issue, I feel very strongly that we should implement #15241. If we want to add the naming of interpreter contraints, then we should do it for a usability reason rather than a performance reason, because I'm confident that #15241 would resolve the performance concern. |
We would still need to iterate over O(n) where n is # targets, vs. O(1) of accessing |
@benjyw you spent a lot of time with interpreter constraints & parametrize when recently adopting Pants in a repo. Purely from UX perspective, what do you think about the constraints being named? (#12652 (comment) for example) |
Purely from a UX perspective, this would be excellent to have. Right now I'm writing macros to get around this. Imagine a repo that builds a wheel for 5 different interpreter versions, and therefore also has to run tests on those versions, for example. Being able to DRY and then parametrize on the interpreter constraint name instead of the actual constraint string would be a great step. |
Awesome! So then there's sufficient motivation:
Now just to figure out how to deprecate... |
IMO, we should probably wait until we know what #13767 will look like. If the configuration idea pans out, then it is roughly equivalent to named ICs, but with a different UX and no deprecation cycle. |
Yeah, I don't know if I would single out interpreter constraints for special UX consideration over using named configurations in general |
Right now, anyone can add an arbitrary interpreter constraint to any Python target, like
interpreter_constraints=["==2.7.*"]
. As explained in https://blog.pantsbuild.org/python-3-migrations/, it's powerful that it's decentralized to set interpreter constraints. But it has downsides:This is an improvement over #12495.
Modeling
There will be an option like this:
Then targets will set:
We also probably still want to support using only a single global interpreter constraint like this, in which case you would not be able to set the
interpreter_constraints
field:It's not clear the best way to model the
[python-setup]
option. We want a mapping of names to constraints. In TOML, that would suggest using a nested table, but that does not work well with our options system which requires usinglist
ordict
.dict
types are awkward in Pants and it would mean we can't keep allowing the simple case of setting a single global interpreter constraint:I think a
list[str]
makes sense, where it can either be a single element w/ just the global constraints, or a list ofkey: value
elements to create a mapping of blessed constraints. If you are doing the blessed constraints approach, we should probably special case the "default" key to be the default. (Is it an error to not set a default?)How to handle deprecation
This is tricky because
interpreter_constraints
is already claimed and is a good name to use.I propose we still keep the same option and field name, and teach our code to handle the old semantics and new one. For example, if the field
interpreter_constraints
is being set to alist[str]
instead ofstr
, assume they're using old semantics and give a deprecation. If astr
, assume new semantics and try to find the named interpreter constraints.The text was updated successfully, but these errors were encountered: