Repository navigation
Conversation
Closed
Member
|
Pushed up again as #10683 to trigger internal CI job: https://github.com/aws/aws-cli/actions/runs/35869196476/job/107208501959?pr=10683, but will review and merge here. |
ashovlin
reviewed
Sep 23, 2026
felixonmars
added a commit
to felixonmars/archriscv-packages
that referenced
this pull request
Oct 4, 2026
Cap pytest-xdist at four workers while retaining automatic selection on smaller builders and the complete existing check suite. The native 64-CPU centiskorch build was terminated by signal KILL during check(); its journal confirms systemd-oomd selected the container after memory pressure exceeded 45% for more than 15 seconds, with a 115.9G memory peak. Unbounded -n auto duplicates the memory-heavy test collection across workers. Upstream aws/aws-cli#10679 addresses test collection memory but remains an unmerged CLI v2 refactor, including fixtures absent from 1.46.1. No directly applicable v1 fix was found; limit concurrency rather than backporting that broad refactor or excluding tests. Retain the unpublished QEMU-user blacklist entry: the earlier electivire check failed three vendored botocore memory tests with RSS growth of 4857856, 4976640, and 4964352 bytes against a 4 MiB limit. Their ps -o rss measurements include QEMU-user allocations rather than only guest Python memory. Keep the assertions unchanged and select native builders. The Python GC regression in botocore #3719 was already fixed before the installed Python 3.14.7. https://archriscv.felixc.at/.status/logs/aws-cli/aws-cli-1.46.1-1.log aws/aws-cli#10679 https://github.com/aws/aws-cli/blob/1.46.1/tests/functional/test_shadowing.py https://github.com/aws/aws-cli/blob/1.46.1/tests/functional/botocore/leak/test_resource_leaks.py https://github.com/aws/aws-cli/blob/1.46.1/tests/utils/botocore/__init__.py pytest-dev/pytest-xdist#1219 boto/botocore#3719 https://github.com/qemu/qemu/blob/v11.1.1/linux-user/syscall.c
ashovlin
previously approved these changes
Oct 27, 2026
ashovlin
left a comment
ashovlin
left a comment
Member
There was a problem hiding this comment.
Looks good once conflicts are addressed.
Model validation retained clients, command tables, and model copies during collection, while operation-level parametrization inflated the test count. Collect service or command cases and build transient state during execution so it can be released between cases. Grouped checks still report the individual operations and shapes that fail, so the set of validations performed is unchanged. Reuse shared service models and parsed documentation to avoid repeated work, and skip redundant logger cache invalidations during cleanup.
Vanity URL support made `aws configure sso` resolve any start URL that isn't an AWS-owned domain by following redirects over the network. Most tests in `test_sso.py` use placeholder start URLs such as `https://starturl`, so every one of them made a real HTTP request that failed at DNS and fell back to the region prompt. The tests passed, but each one waited for the lookup to fail, which took about eight seconds apiece on some machines, and their behavior depended on the network they ran on. An autouse fixture now makes `resolve_start_url` raise the same `ConfigurationError` the real resolver raises on failure. The tests exercise the same fallback path as before, without network access. The tests that cover vanity URL resolution already patch `resolve_start_url` themselves, and those patches override the fixture.
`AuthCodeFetcher` starts an `http.server.HTTPServer` for the OAuth callback. When the server binds, the standard library calls `socket.getfqdn`, which does a reverse DNS lookup of the local hostname. On machines where that hostname can't be resolved locally, the lookup waits for a DNS timeout of about five seconds. Every login test that used the same-device flow and every `AuthCodeFetcher` test paid that wait, even though nothing reads the server name the lookup produces. The login functional tests already mock `fetch_token`, so they never use the callback server. They now mock `AuthCodeFetcher` outright, as the `sso login` functional tests already do. The `AuthCodeFetcher` unit tests need a real server to send requests to, so they patch only `socket.getfqdn` while the server is created. The server still binds a real port and handles real requests. The same lookup happens when users run `aws login` or `aws sso login` with the browser flow. This commit changes only the tests.
The internal build system parses the JUnitXML report of tests marked `validates_models`. It expects each testcase to record one `aws_service` and `aws_operation` pair so that each failure maps to a single operation. c1a691e grouped the event-stream and shadowing checks by top-level command and the completions check by service, then called `record_property` inside a loop, so one testcase could record several pairs. The event-stream test also recorded properties for allow-listed operations before checking for failures. A new streaming operation on s3 would have been reported as SelectObjectContent. The current report avoids this only because `removals.py` removes every other event-stream operation. This is the same regression e6b7544 fixed in January. Each testcase is again one parametrized service and operation. The parameters now come from the botocore models in `ALL_SERVICES` instead of from walking the CLI command table at collection time. Every xdist worker collects every file, so collection work repeats in each worker. Removing that walk was the goal of c1a691e. Each test builds the command table for only the service it checks. The shadowing test parametrizes over every operation and indexes each service on first use with a fresh driver, so command tables do not accumulate across services. The completions test parametrizes over violations and emits one skipped placeholder when there are none, as it did before c1a691e. Mapping a botocore service to its top-level command requires the public `service_model` property, which loads that service's model. Loading it for all 431 commands uses about 400MB per process. Instead, a command whose name matches a botocore service is assumed to be bound to that service, and `iter_operation_commands` checks the binding before yielding. Only commands with a different name, such as `s3api`, load a model. If the assumption is wrong, `iter_operation_commands` raises `RuntimeError` and the test fails. An operation the CLI does not expose passes without recording properties, so the report lists only operations that were checked. An autouse fixture in `tests/conftest.py` fails any `validates_models` test that records a property name twice. A change that reintroduces this regression now fails in the test suite instead of producing a report that misattributes failures.
jamesls
force-pushed
the
test-mem-cleanup
branch
from
November 3, 2026 14:09
0bb5b62 to
4fe53eb
Compare
The example style checks added recently made test_examples.py run about four times longer and use about 600 MB more memory. Most of the cost came from the validation setup, not from the checks themselves. With this change, running test_examples.py in a single process went from 5:00 to 1:10 wall clock and from 1,543 MB to 699 MB max RSS on a 2-CPU Linux machine. That is about 4.3x faster and less than half the memory, and below what the file used before the style checks were added. Each `--cli-input-json`, `--cli-input-yaml` and `--generate-cli-skeleton` argument registers its required-args handler for the event of every command when its argument table is built. The CLI builds one argument table per run, but these tests build thousands, so the stale handlers piled up and each `emit` walked millions of them. `_cli_required_args` now unregisters them and calls only the current command's handlers directly, which matches what the CLI sees. The `command_table` and `command_validator` fixtures each created a driver, and each driver loaded every service model it touched, so every model was held in memory twice for the whole module. `ServiceCommandTable` now loads one service at a time in a fresh driver and releases the previous one. The examples are grouped by service directory and very few refer to another service, so drivers are rarely rebuilt. The ten per-rule style tests are merged into one `test_example_style` per file, so each file is read and block-parsed once instead of ten times. It still runs every rule and reports every rule that fails, so coverage is unchanged, but failures no longer have separate per-rule test IDs. Two smaller costs are gone as well. The docutils `OptionParser` was rebuilt for every file, which took longer than parsing most examples, so it is now cached. A module-scoped `gc.freeze()` stops full garbage collections from rescanning the roughly one million objects left behind by test collection.
Member
Author
|
Rebased and fixed conflicts. One thing I noticed is that some of recent PRs to the functional tests for validating the doc examples moved things the other direction and increased total run time and memory. Comparing the original version before and after the rebase today I'm seeing, just for To propose something concrete, I pushed another commit, 94d7a4b, which attempts to address this. It batches the tests to together to consolidate them all back down to the original counts (from 70k tests back down to around 17k tests), while still running all the checks for each file and having an individual test failure have multiple violations flagged. I know there were some issues before about the granularity of reporting so hopefully this still works for you all. With these changes, I'm now seeing: Let me know what you think, happy to explore any other approaches that would bring down execution time / memory. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are refactorings to the existing tests that should have no
change in overall test coverage. These changes bring down the total
max memory usage and total invocation time of the
scripts/ci/run-testsused in GitHub actions.
I ran into the GHA
run-testsjob being cancelled due to resource usageerrors. On my dev machines I was seeing upwards of 16 GiB+ max rss. With these
changes, I'm seeing around 7 GiB max rss, and the total time for
scripts/ci/run-tests --with-covto just over 5 minutes. Interestingly, thetotal runtime on the GHA still take around 14-16min which suggests to me that
we're swapping and that this usage is still too high. However, the total
reduction in max rss though should make these test runs less susceptible to
cancellation due to OOM.
As for the changes themselves, the model validation now collects one case per
service or command and builds the state it needs during execution, so it can be
released between cases. Grouped checks still report every operation and shape
that fails, so the validations performed are unchanged. The visible differences
are coarser test IDs and that a single test may record several
aws_operationproperties. Tests that loaded their own service models reuse the shared
ALL_SERVICESlist, and the RST and streaming body docs tests no longer parseor generate the same documents more than once.
For the improvements to total wall clock time, I picked off the top slowest
tests for now, which were the SSO tests. They were making network requests to
placeholder start URLs but were expecting a fallback anyways. This added 8+
seconds stalls for each of those tests. This brought their run time from 3+min
down to about 4 seconds. Similar type of change to the login
functional tests with the 5+ second stalls waiting for
socket.getfqdn.