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.

Sign up for GitHub

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

Jump to bottom

We semi-broke partitioners for targets #18566

Closed
thejcannon opened this issue Mar 23, 2023 · 1 comment · Fixed by #18725
Closed

We semi-broke partitioners for targets #18566

thejcannon opened this issue Mar 23, 2023 · 1 comment · Fixed by #18725
Assignees
@danxmoran
Labels
bug
Milestone
2.15.x

Comments

@thejcannon
Copy link
Member

In all of our partitioners for fix/lint/fmt/test we stringify field sets for the partition key, which for some reason ends up calling repr which fails if a field doesn't have a default.

We should using .address on field sets. I think we just got lucky today with our current field sets 😅

CC @danxmoran
@thejcannon thejcannon added the bug label Mar 23, 2023
@thejcannon thejcannon added this to the 2.15.x milestone Mar 23, 2023
@thejcannon
Copy link
Member Author

Correction: str(.address)

@danxmoran danxmoran self-assigned this Mar 23, 2023
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 12, 2023
@danxmoran
Fix repr for Fields that don't define default.
010085e 
Found while working on pantsbuild#18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
@danxmoran danxmoran mentioned this issue Apr 12, 2023
Merged
danxmoran added a commit that referenced this issue Apr 12, 2023
@danxmoran
Fix repr for Fields that don't define default. (#18719)
8ced7d2 
Found while working on #18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
@danxmoran danxmoran mentioned this issue Apr 12, 2023
Merged
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 12, 2023
@danxmoran
Fix repr for Fields that don't define default. (pantsbuild#18719)
f774be1 
Found while working on pantsbuild#18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
@danxmoran danxmoran mentioned this issue Apr 12, 2023
Merged
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 12, 2023
@danxmoran
Fix repr for Fields that don't define default. (pantsbuild#18719)
128bda5 
Found while working on pantsbuild#18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 12, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test.
d8753cc 
Closes pantsbuild#18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. This previously raised an exception in some cases - it's
also an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x. Revert back to using `.address` as the
partitioning key for field sets.
@danxmoran danxmoran mentioned this issue Apr 12, 2023
Merged
danxmoran added a commit that referenced this issue Apr 12, 2023
@danxmoran
Fix repr for Fields that don't define default. (Cherry-pick of #…
575c85f 
…18719) (#18723)

Found while working on #18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
danxmoran added a commit that referenced this issue Apr 13, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test (#…
21b992e 
…18725)

Closes #18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. After #18719 this no longer raises an exception, but
it's still an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x.

Revert back to using `.address` as the partitioning key for field sets,
and add regression tests.
@danxmoran danxmoran mentioned this issue Apr 13, 2023
Merged
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 13, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test (p…
0e1f5c0 
…antsbuild#18725)

Closes pantsbuild#18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. After pantsbuild#18719 this no longer raises an exception, but
it's still an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x.

Revert back to using `.address` as the partitioning key for field sets,
and add regression tests.
@danxmoran danxmoran mentioned this issue Apr 13, 2023
Merged
danxmoran added a commit to danxmoran/pants that referenced this issue Apr 13, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test (p…
772c9a5 
…antsbuild#18725)

Closes pantsbuild#18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. After pantsbuild#18719 this no longer raises an exception, but
it's still an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x.

Revert back to using `.address` as the partitioning key for field sets,
and add regression tests.
danxmoran added a commit that referenced this issue Apr 13, 2023
@danxmoran
Fix repr for Fields that don't define default. (Cherry-pick of #…
a00ceed 
…18719) (#18724)

Found while working on #18566

The code for `Field` declares that subclasses must define one of
`default` and `required`, and leaves `default` without a value. However
its implementation of `__repr__` assumes that `default` will always be
defined. This can cause errors like:
```
AttributeError: 'RequiredField' object has no attribute 'default'
```
In various places, i.e. when stringifying a `FieldSet` or when
converting a `Target` to a `FieldSet` via `field_set_type.create(tgt)`.
danxmoran added a commit that referenced this issue Apr 13, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test (…
204477a 
…Cherry-pick of #18725) (#18733)

Closes #18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. After #18719 this no longer raises an exception, but
it's still an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x.

Revert back to using `.address` as the partitioning key for field sets,
and add regression tests.
danxmoran added a commit that referenced this issue Apr 13, 2023
@danxmoran
Use address as stable key when batching field sets in lint/test (…
9e7efee 
…Cherry-pick of #18725) (#18734)

Closes #18566

Using `str(x)` as the stable batching key ends up using `repr` when `x`
is a field set. After #18719 this no longer raises an exception, but
it's still an unintended behavior change introduced as part of the new
partitioning logic in 2.15.x.

Revert back to using `.address` as the partitioning key for field sets,
and add regression tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danxmoran danxmoran

Labels
bug
Projects
None yet
Milestone
2.15.x
Development

Successfully merging a pull request may close this issue.

Use address as stable key when batching field sets in lint/test danxmoran/pants
2 participants
@danxmoran @thejcannon