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

Fix issue with explicit dependency when using parametrize groups. #20768

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

kaos
Copy link
Member

@kaos kaos commented Apr 8, 2024

When both a target and a explicitly provided dependency is using parametrize groups (the **parametrize(..) syntax), and the dependency does not explicitly provide the parametrization to use, pick the matching parametrization group with the target if available, rather than just the first one.

Closes #20739

@kaos kaos added the category:bugfix Bug fixes for released features label Apr 8, 2024
@kaos kaos added this to the 2.20.x milestone Apr 8, 2024
@kaos kaos requested a review from huonw April 8, 2024 11:29
@kaos
Copy link
Member Author

kaos commented Apr 8, 2024

Could pick to 2.19.x if we are still running releases for that line..?

@huonw huonw modified the milestones: 2.20.x, 2.19.x Apr 9, 2024
@huonw
Copy link
Contributor

huonw commented Apr 9, 2024

I don't understand the mechanics here in detail... but what happens if the parametrizations are slightly different, with different names or fields. For instance, here's a semi-plausible example:

python_sources(
  ...,
  **parametrize("a", resolve="a", skip_mypy=True), # mypy doesn't work in this resolve
  **parametrize("b", resolve="b", skip_mypy=False),
)

python_tests(
   ...,
  # Want to run the tests in a few different environments, so different name & more fields than the source
  **parametrize("a-1", resolve="a", skip_mypy=True, environment="some_docker"),
  **parametrize("a-2", resolve="a", skip_mypy=True, environment="another_docker"),
  # fewer fields than the source, because didn't add the skip_mypy=False explicitly
  **parametrize("b", resolve="b"),

)

Could pick to 2.19.x if we are still running releases for that line..?

Might as well 🤷‍♂️ releases are easy enough now.

@kaos
Copy link
Member Author

kaos commented Apr 9, 2024

I don't understand the mechanics here in detail... but what happens if the parametrizations are slightly different, with different names or fields. For instance, here's a semi-plausible example:

python_sources(
  ...,
  **parametrize("a", resolve="a", skip_mypy=True), # mypy doesn't work in this resolve
  **parametrize("b", resolve="b", skip_mypy=False),
)

python_tests(
   ...,
  # Want to run the tests in a few different environments, so different name & more fields than the source
  **parametrize("a-1", resolve="a", skip_mypy=True, environment="some_docker"),
  **parametrize("a-2", resolve="a", skip_mypy=True, environment="another_docker"),
  # fewer fields than the source, because didn't add the skip_mypy=False explicitly
  **parametrize("b", resolve="b"),

)

We only match on the a vs a-1 and b here (i.e. the "name" of the parametrization group), not the fields. (if you go to the source and see field value matching stuff, that's for the "regular" field parametrizations like resolve=parametrize("one", "two"))
So it would pick the "first" one when there is not a matching group on both ends. (which is the same behavior as now.)

As the actual parameter values are not part of the address for grouped parametrizations, we don't know them as we only work with addresses for the matching, so this is the best we can do, currently, I think (with little effort). (or at least, it's a good first step which we can follow up on to improve later if we want..)

Could pick to 2.19.x if we are still running releases for that line..?

Might as well 🤷‍♂️ releases are easy enough now.

✔️

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would not be a fun bug to run into! This fix makes sense to me.

@kaos kaos merged commit 8b88837 into main Apr 12, 2024
24 checks passed
@kaos kaos deleted the kaos/20739 branch April 12, 2024 07:20
WorkerPants pushed a commit that referenced this pull request Apr 12, 2024
…0768)

When both a target and a explicitly provided dependency is using
parametrize groups (the `**parametrize(..)` syntax), and the dependency
does not explicitly provide the parametrization to use, pick the
matching parametrization group with the target if available, rather than
just the first one.

Closes #20739
WorkerPants pushed a commit that referenced this pull request Apr 12, 2024
…0768)

When both a target and a explicitly provided dependency is using
parametrize groups (the `**parametrize(..)` syntax), and the dependency
does not explicitly provide the parametrization to use, pick the
matching parametrization group with the target if available, rather than
just the first one.

Closes #20739
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.19.x

Successfully opened #20778.

✔️ 2.20.x

Successfully opened #20779.


Thanks again for your contributions!

🤖 Beep Boop here's my run link

kaos added a commit that referenced this pull request Apr 15, 2024
…erry-pick of #20768) (#20778)

When both a target and a explicitly provided dependency is using
parametrize groups (the `**parametrize(..)` syntax), and the dependency
does not explicitly provide the parametrization to use, pick the
matching parametrization group with the target if available, rather than
just the first one.

Closes #20739

Co-authored-by: Andreas Stenius <[email protected]>
kaos added a commit that referenced this pull request Apr 17, 2024
…erry-pick of #20768) (#20779)

When both a target and a explicitly provided dependency is using
parametrize groups (the `**parametrize(..)` syntax), and the dependency
does not explicitly provide the parametrization to use, pick the
matching parametrization group with the target if available, rather than
just the first one.

Closes #20739

Co-authored-by: Andreas Stenius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parametrized python_tests and python_sources in same package not lining up when using explicit dependencies
4 participants