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

Improve type annotations in core functions #850

Merged
merged 44 commits into from
Jun 25, 2024

Conversation

achimnol
Copy link
Member

@achimnol achimnol commented May 20, 2024

refs #847

  • Apply from __future__ import annotations.
  • Explicitly type most core APIs (containers, images, logs).
    • Add overloaded type definitions to DockerContainer.log(), DockerContainer.stats() like DockerImage.pull().
    • Introduce JSONObject, JSONList, JSONValue for API arguments, but not in the return values due to the type variance issue. In the future, it would be better to adopt explicitly typed DTOs.
    • Clearly define the types for the filters arg used in many places.
  • Explicitly type most test codes.
  • Fix several hidden bugs or missing handling of corner cases via improved type annotations.
  • Drop compatibility codes for Python 3.7 or older versions.
  • Improve the flaky Windows tests in CI.
    • The official "python:3.12" (or "python:latest") image for the windows/amd64 platform weighs more than 2 GiB, while the alpine version is less than 20 MiB. This makes it to time out in high probability if pulled in pytest, as the pull takes more than 10 minutes. Unfortunately, there is no slim variants for the windows/amd64 platform.
    • Split out and run the docker pull step explicitly before running pytest in CI, so that we can observe the pulling progress and decouple the timeout from pytest.
    • Tried to apply Docker image caching, but found that the bottleneck of GitHub Actions is disk I/O. docker load and docker save simply takes a tooooo long time, so it's meaningless to improve the download speed by caching.

Note

#861 has introduced explicit cancellation of utils._AsyncCM, but this PR removes it completely and replaces it with the intrinsic async context managers which provides better exception handling as "originally" expected.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the changes folder
    • name it <issue_id>.<type> for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 84.89209% with 21 lines in your changes missing coverage. Please review.

Project coverage is 81.14%. Comparing base (7b3d86f) to head (d6ccbe0).

Files Patch % Lines
aiodocker/logs.py 11.11% 8 Missing ⚠️
aiodocker/docker.py 69.56% 4 Missing and 3 partials ⚠️
aiodocker/containers.py 85.00% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
- Coverage   81.25%   81.14%   -0.12%     
==========================================
  Files          24       24              
  Lines        1382     1421      +39     
  Branches      245      253       +8     
==========================================
+ Hits         1123     1153      +30     
- Misses        173      175       +2     
- Partials       86       93       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This will improve typing when we accept user-provided arguments.
Though, it mandates our users to annotate/cast the return values
explicitly due to type variances.  To completely resolve this issue,
we have to introduce typed DTOs using pydantic or msgspec.
- `utils._AsyncCM` is completely removed and replaced with the intrinsic
  `contextlib._AsyncGeneratorContextManager`.
- If they are used in return values, the caller should perform extra
  type-narrowing or casting to pick a final type from union to avoid
  type check failures.
- In a long term, it is better to declare typed DTOs.
@achimnol achimnol marked this pull request as ready for review June 25, 2024 07:24
@achimnol
Copy link
Member Author

achimnol commented Jun 25, 2024

Currently the Windows test in CI seems to be broken in an unexpected way.
In my local Windows box (standard Docker Desktop + Python 3.12.4 from chocolatey), it works well with a minor glitch:

image image

@achimnol achimnol merged commit dda48e6 into master Jun 25, 2024
12 checks passed
@achimnol achimnol deleted the refactor/improve-type-anno-for-query branch June 25, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant