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

integrate with lower-bound-checker to ensure constraints files stay in sync with setup.py #1974

Open
tswast opened this issue Jul 10, 2024 · 7 comments
Assignees
Labels
api: bigquery Issues related to the googleapis/python-bigquery API.

Comments

@tswast
Copy link
Contributor

tswast commented Jul 10, 2024

See: googleapis/python-test-utils#8 and #1972 (comment)

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jul 10, 2024
@Linchin
Copy link
Contributor

Linchin commented Jul 10, 2024

I'm not sure if it's a good idea, but we can add lower-bound-checker update to owlbot.py to ensure constraints-3.x.txt are updated for each commit. WDYT? @tswast

@tswast
Copy link
Contributor Author

tswast commented Jul 12, 2024

That could work. I would expect owlbot to generate the constraints and noxfile to check them.

I'm not sure how well the generation works, but if it does let's use it.

@chalmerlowe
Copy link
Collaborator

I walked through the code in that PR.
There is a lot of code across multiple files (close to 700 lines).

I am presuming we are considering importing this google-cloud-testutils library during our CI/CD checks rather than adding all that code to our repo?

If yes, we should also mirror that same process to the other repos that have lower bounds in their constraints files.

As for doing the checks... These folks have two separate nox sessions in their nox file:

  • check_lower_bounds
  • update_lower_bounds

The check_lower_bounds session runs every time.
The update session is run locally by the developer to update the lower bounds when needed, same way we might run blacken OR lint locally before pushing a commit.

@chalmerlowe chalmerlowe self-assigned this Jul 29, 2024
@tswast
Copy link
Contributor Author

tswast commented Jul 29, 2024

I am presuming we are considering importing this google-cloud-testutils library during our CI/CD checks rather than adding all that code to our repo?

We already install google-cloud-testutils in most of our nox sessions if I remember correctly, so that would be the correct approach.

The check_lower_bounds session runs every time.

Yes, I think the lint for the setup.py file would be a good spot for that.

The update session is run locally by the developer

I was thinking owlbot could do this too, just like we run our blacken session

s.shell.run(["nox", "-s", "blacken"], hide_output=False)

@chalmerlowe
Copy link
Collaborator

chalmerlowe commented Jul 30, 2024

The way the current code functions, lines such as this in setup.py break the processing:

"protobuf>=3.20.2,<6.0.0dev,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4",

Lines with two endpoints is fine, but adding the extraneous versions fails.

"protobuf>=3.20.2,<6.0.0dev"

Order does not matter. Putting the 4.* versions between the two endpoints does not help. Similarly, putting spaces between the values after each comma makes no difference.

"protobuf>=3.20.2,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,<6.0.0dev",

@chalmerlowe
Copy link
Collaborator

chalmerlowe commented Jul 30, 2024

@tswast do you have strong feelings about this Issue?

Some more context:

Their _lower_bound() function has this in the docstring:

    The lower bound can be determined for a requirement only if it is one of these
    formats:
        foo==1.2.0
        foo>=1.2.0
        foo>=1.2.0, <2.0.0dev
        foo<2.0.0dev, >=1.2.0

It may be possible to issue a PR (or request this feature) to add something to their code to account for more complex situations... there would need to be an accounting for edge cases, (are the != versions in the middle of the pack, at the end, at the beginning) etc.

Would need to include tests.

Another option is to not have requirements that are that complex, but sometimes we can't get around it (especially when there is a broken dependency that we need to weed out). In a check of the other repos in googleapis, a declaration to not use a specific version shows up ~450 times. Typically, but not exclusively, relative to google-api-core, google-auth, 'protobuf`.

@tswast
Copy link
Contributor Author

tswast commented Jul 31, 2024

do you have strong feelings about this Issue?

I think we've had a couple times in the last year of setup.py and constraints getting out of sync. These cause real customer issues where they end up with incompatible versions because we didn't test with our minimum advertised dependency versions.

IMO some automation is a high priority to prevent such errors in future. A contribution to google-cloud-testutils seems the right approach, but certainly there are alternatives like using the uv package manager to install instead of pip, since uv has a flag to install minimum supported versions instead of latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API.
Projects
None yet
Development

No branches or pull requests

4 participants