-
Notifications
You must be signed in to change notification settings - Fork 520
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 a script to populate parts of tox.ini #3920
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3920 +/- ##
==========================================
- Coverage 80.21% 80.15% -0.07%
==========================================
Files 139 139
Lines 15394 15394
Branches 2596 2596
==========================================
- Hits 12349 12339 -10
- Misses 2202 2206 +4
- Partials 843 849 +6
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started reviewing this PR – I am fully on board with the idea of automating our tox
setup, but this PR is quite large and difficult to review.
Would you be open to splitting this into multiple smaller PRs? I think, for example, the changes to the Python versions in the .github
directory could be in their own PR, adding the script could possibly also be its own thing, and we might even consider making PRs for each individual integration we move over to the script (that level of division might admittedly be overkill though if moving an individual integration to the script is a trivial change)
@@ -45,7 +45,10 @@ jobs: | |||
python-version: 3.12 | |||
|
|||
- run: | | |||
pip install -e . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question (not related to this PR): Have we thought about using uv pip
here? In my experience, it is much faster than pip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3920 (comment), we can consider this, but not in this PR.
@@ -29,7 +29,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python-version: ["3.7","3.9","3.11","3.12","3.13"] | |||
python-version: ["3.7","3.9","3.11","3.12"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing 3.13 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3920 (comment) (also for the other CI YAML changes)
@@ -22,70 +22,6 @@ env: | |||
CACHED_BUILD_PATHS: | | |||
${{ github.workspace }}/dist-serverless | |||
jobs: | |||
test-flags-latest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain why we need to delete these lines (and the lines in the other files)? It is unclear to me from the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation here: #3974 (comment)
Basically, the script doesn't generate a -latest
target anymore, just pinned versions. This is fine since it always makes sure to include the latest version out there. So if we run this script often enough (which is the plan), it effectively substitutes the -latest
category.
@@ -29,7 +29,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python-version: ["3.8","3.9","3.11","3.12","3.13"] | |||
python-version: ["3.9","3.12","3.13"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing 3.8 and 3.11?
@@ -29,7 +29,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python-version: ["3.6","3.7","3.8","3.9","3.11","3.12","3.13"] | |||
python-version: ["3.8","3.9","3.11","3.12","3.13"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove versions here, but add them later in the file?
pip install -e .. | ||
pip install -r populate_tox/requirements.txt | ||
pip install -r split_tox_gh_actions/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same general idea of using uv pip
instead of pip
(just wondering if it is possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably is possible but would be a bit of a larger overhaul. I think @antonpirker was already looking into switching to uv and was also happy about the performance. So this is something we can definitely consider.
dependencies, optionally gated behind specific conditions; and optionally | ||
the Python versions to test on. | ||
|
||
The format is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider formalizing these formats as TypedDicts, or by using some other structured data type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this in a follow up PR, might be nice.
} | ||
|
||
|
||
@functools.cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally it creates a dict of args -> return value that it then uses as a cache on consequent calls. Here it helps us avoid making extra API calls. See https://docs.python.org/3/library/functools.html#functools.cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more feedback
min_supported = Version(".".join(map(str, min_supported))) | ||
else: | ||
print( | ||
f" {integration} doesn't have a minimum version defined in sentry_sdk/integrations/__init__.py. Consider defining one" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this show up? Is it meant to be run locally, or in a GitHub action? Just want to make sure it is not too easy to miss this message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal would eventually be to run this in a GH action. The log is mostly informative, if we notice it at some point and have the bandwidth, we can act on it. Not a big deal if we miss it. If we at some point decide we care more, we can figure out ways to make it more prominent.
else: | ||
expected_python_versions = SpecifierSet(f">={MIN_PYTHON_VERSION}") | ||
|
||
def _supports_lowest(release: Version) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be an inner function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used as the key
function for the bisect
couple lines after, and nowhere else.
len(releases) // 3, | ||
len(releases) // 3 * 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment might be helpful here, to describe what these are trying to do.
Also, maybe we should use a set
for indexes
instead of a list
, since depending on the value of releases
, this could have duplicates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add a comment in #3971
The indexes might be duplicates, but they're only used for building the filtered_releases
set, where any duplicates will be ignored. If indexes
was a huge list we could optimize but in this case it doesn't really make a difference.
jinja2 | ||
packaging | ||
requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should pin these to specific versions to avoid breaking the script in the event of a breaking change in these packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine to leave as is. We're only using very basic public APIs. If anything changes we can pin the version then.
Closing this in favor of #3971 & friends to avoid confusion, but will get back to you @szokeasaurusrex regarding the comments |
@szokeasaurusrex Regarding the CI YAML changes, these are still generated automatically by the The Python changes, in general, are due to the script auto-determining the Python versions to use from the package's metadata on PyPI. I think, for instance, we were testing some packages on 3.13 before they were officially supporting it (= they had 3.13 in their PyPI classifiers). The script also auto-selects a handful of Python versions to actually test and this is now automated instead of done manually (and possibly inconsistently), also leading to CI YAML changes. |
Right now, our
tox.ini
has to be kept up-to-date with new releases manually. This is not feasible as we have tens of test suites.This is the first step towards fully automating the process. This PR adds a script that's capable of figuring out which releases of a package are available, are supported, and should be tested.
How it works
For each package, test the minimum supported version (defined by
MIN_VERSION
inintegrations/__init__.py
, the Python versions supported by both the SDK and the package, and the date of the release -- only packages not older than 5 years will be considered).Additionally, if the package has majors, test the last version of each major as well as the first release in the last major.
If the package doesn't have majors, take the first and last supported release and two in between.
The future
Currently, the script is only responsible for generating the
tox.ini
entries for ~28 of our integration test suites.More to follow in follow up PRs.
Ideally at some point the Tox entries for all integrations are generated by this script. We can then also get rid of the
-latest
test suites once the script is running and regeneratingtox.ini
on an automated basis (since it'll catch newest releases and add them totox.ini
on its own).Relates #3808