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

tooling: Add requirements.in and rationalize deps #17984

Closed
wants to merge 2 commits into from

Conversation

phlax
Copy link
Member

@phlax phlax commented Sep 3, 2021

Signed-off-by: Ryan Northey [email protected]

Commit Message: tooling: Add requirements.in and rationalize deps
Additional Description:

This adds a requirements.in file to list actual deps to create the requirements.txt file from, which will make it much easier to manage pip deps

It also removes several requirements targets that are not necessary

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 3, 2021
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17984 was opened by phlax.

see: more, trace.

@phlax phlax requested review from moderation and htuch September 3, 2021 18:31
@phlax phlax force-pushed the tooling-requirements-in branch 8 times, most recently from 05cfede to adcfb65 Compare September 3, 2021 20:15
@phlax phlax force-pushed the tooling-requirements-in branch from adcfb65 to 19c7645 Compare September 3, 2021 20:46
@htuch
Copy link
Member

htuch commented Sep 5, 2021

@phlax please try split PRs like this in the future into trivial (i.e. the requirements.in) which can be rubber stamped and the unrelated deps rationalization.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

@moderation for comment.

@@ -0,0 +1,5 @@
coverage
Copy link
Member

Choose a reason for hiding this comment

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

We're not picking specific versions any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

not in the requirements.in file unless we have a specific reason to - the requirements.txt file is generated from these and that does lock/pin/pick specific versions

that said we can set versions here if we need to and pip-compile should respect them (not sure about dependabot)

Copy link
Member Author

Choose a reason for hiding this comment

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

some further investigation of using pip-compile

one annoying issue is that it uses sets and doesnt order the hashes, which means running it is non-deterministic (ie the hashes keep reordering) - which is a pita 8/. I had thought that this problem was due to running different versions of pip-compile, but now it seems like the issue is more as described.

the other thing which doesnt work so well with pip-compile is conditional deps

if you set a conditional dep on eg sys_platform = "win32" or somesuch pip-compile respects it in the sense that if you are not on windows it doesnt get included in the requirements.txt file

this is pretty rubbish for mananging repo requirements if you expect different install/run environments

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@phlax phlax Sep 7, 2021

Choose a reason for hiding this comment

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

this is already an issue as windows seems to be expecting pyreadline in some of my prs

we can just add it as dep, currently that would make linux pull it in unnecessarily, but that also could be resolved with (the new) pip_install_incremental in bazel

the problem with this approach occurs where there is a dep for some particular env that you really dont want to be pulled in, but im not sure that is an issue for envoy. In that case the best thing would be to separate the deps in bazel i think

tools/testing/requirements.txt Outdated Show resolved Hide resolved
@phlax
Copy link
Member Author

phlax commented Sep 5, 2021

... and the unrelated deps rationalization.

its not really unrelated, i added the pins so that i could do the rationalization - what i was reluctant to do was add loads of pins, and requirements file updates (which would have been just as large a pr) when rationalizing just got rid of the files altogether

i have a few prs in waiting to further tidy up and rationalize tooling, they all intersect, which is why i pushed this up front, to get rid of anything that would otherwise be repeatedly changed

Signed-off-by: Ryan Northey <[email protected]>
@@ -4,7 +4,6 @@
import re
import subprocess
import fileinput
from six.moves import input
Copy link
Member Author

@phlax phlax Sep 6, 2021

Choose a reason for hiding this comment

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

if we want to keep six here we could depend on it from elsewhere (ie base_pip3)

i would prefer not to. Python >= 3.8 is an assumption baked into a lot of the tooling (even tho much will work with varying mileage with less)

the pytooling repo tests against 3.8 and 3.9 currently

@phlax
Copy link
Member Author

phlax commented Sep 8, 2021

#18026 provides a better solution for this as it rationlizes the requirements down to a single file by using the new pip_install_incrememental

im therefore closing this in favour of that pr

@phlax phlax closed this Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants