GeezerVison is showing github.com — Complete PyTime conversion fixtures and repair existing helpers by youknowdot · Pull Request #9317 · RustPython/RustPython
Conversation
youknowdot
commented
Oct 22, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Complete the coherent 11-helper PyTime conversion family in _testinternalcapi:
- Repair the three existing helpers:
_PyTime_AsTimeval_clamp, _PyTime_AsTimespec, and _PyTime_AsTimespec_clamp. Honor all four rounding modes, negative values, ties-to-even, carries, native bounds, and CPython wrapper argument validation/precedence. Derive SIZEOF_TIME_T from the host type.
- Add the eight real conversion fixtures needed by the canonical PyTime tests:
_PyTime_FromSeconds, _PyTime_FromSecondsObject, _PyTime_AsTimeval, _PyTime_AsMilliseconds, _PyTime_AsMicroseconds, _PyTime_ObjectToTime_t, _PyTime_ObjectToTimeval, and _PyTime_ObjectToTimespec.
- Keep the consumed numeric conversions in a private helper module. Windows timeval seconds use C long independently of 64-bit time_t; clamping clears the subsecond field.
This intentionally extracts the PyTime fixture/API prerequisite from the broader work in #9143 and #8954. The original three-helper repair and the additional eight fixtures are kept together in this PR rather than publishing a duplicate dependent PR. When #9143 is rebased, preserve or reuse these contracts instead of replacing them with less strict wrappers.
No _testcapi namespace, unused shared exports, time.sleep refactor, lifecycle changes, or Lib edits are introduced. These are Python-facing fixture implementations, not a claim of C-ABI coverage.
Validation
Pinned reference: CPython 3.15.0 final, d220409a13b77de7a6c30abe16ea90db01528edc. Oracle interpreter, libpython and _testinternalcapi extension hashes are bound to the retained build/source provenance.
Validated final commit: 29404850a370aad85467a782b65d7b44349a5f62.
- 2017/2017 fixed builtin-only observations for the eight added fixtures match CPython exactly, including typed results, exception types/messages, all rounding modes, signed ties/carries, integer boundaries, exact float inputs, arity/kwargs, invalid builtin modes/types, and conversion precedence. No unavailable helpers.
- 287/287 existing-helper regression observations match CPython. The original baseline had 99 differences (70 tuple values, 7 exception types, 22 messages); all remain resolved with no new differences.
- Normal repository commit hooks pass.
- CLI build with the root
capi feature passes.
- Strict VM Clippy passes with default features and with threading (
-D warnings).
- VM
--no-default-features check passes, with 18 warnings in unchanged code; this is not a warning-free gate.
The original baseline binary was main 0b0a3607 plus the AST-Single warning patch from #9303. Its entire _testinternalcapi.rs matched the PR's main 94f92e4a base, and the exec-mode integer matrix does not exercise that warning route. This is not a claim of exact-current-main baseline or fresh-main integration testing.
Local runtime validation is Linux 64-bit. Integer 32-bit time_t probes and source-reviewed platform branches do not establish Windows/32-bit execution; float neighbors around ±2**31 are not covered. Custom conversion callbacks and subclasses are not exercised by these finite matrices.
Canonical PyTime classes require _testcapi even for the internal helpers. Their guards are unchanged, and skipped classes are not counted as passing coverage. No callback, sleep, subprocess, or lifecycle tests are claimed.
AI assistance was used in preparing this change and its validation.
Summary by CodeRabbit
- Bug Fixes
- Time conversion test helpers now apply the requested rounding mode and report errors for invalid arguments, unsupported rounding values, and out-of-range inputs.
- Clamped conversions return the nearest supported time bound with a zero fractional component.
- Compatibility
- Time-size reporting and conversion limits now reflect platform-specific time types and bounds.
Rebase validation (2026-10-24)
Rebased onto main 1cd1b5d09f4853b2f13a5f3d5d6f856e901b671e as 4aee9e991297300a664593293d05bdb37316acdb. Both original commits are unchanged in git range-diff, and the full-index patch is byte-identical. No Lib, test, sleep, or lifecycle changes were added.
- Normal commit hooks, threaded VM
cargo check, strict threaded VM Clippy (-D warnings), and the default-feature CLI build passed on the exact rebased tree.
- The 12 canonical numeric methods were selected with their existing imports and decorators: all 12 skipped; zero test bodies executed. This supplies compile/lint/build coverage, not numeric behavior validation. Earlier runtime observations above apply to the original commit, not a rerun on this rebased head.
- No full workspace, C-API, release, lifecycle, or cross-platform execution is claimed for this local rebase validation. Correctness CI run 40042903402 completed successfully on this exact rebased head: 25/25 jobs passed.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
chatgpt-codex-connector[bot] reacted with thumbs up emoji
coderabbitai
Bot
commented
Oct 22, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|

Important
Review skipped
Review was skipped as selected files did not have any reviewable changes.
⚙️ Run configuration
-
📥 Commits
-
Reviewing files that changed from the base of the PR and between 2940485 and 4aee9e9.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
-
⚙️ Run configuration
-
📥 Commits
-
Reviewing files that changed from the base of the PR and between 94f92e4 and 2940485.
📒 Files selected for processing: 2
-
crates/vm/src/stdlib/_testinternalcapi.rs
crates/vm/src/stdlib/_testinternalcapi/pytime.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📝 Walkthrough
-
The _testinternalcapi module adds PyTime conversion helpers with platform-aware bounds, overflow checks, and rounding modes. Its test wrappers expose conversions for seconds, timevals, timespecs, subseconds, and objects.
Changes: PyTime test conversion helpers
| Layer / File(s) |
Summary |
Platform types and rounding crates/vm/src/stdlib/_testinternalcapi.rs, crates/vm/src/stdlib/_testinternalcapi/pytime.rs |
The code uses the host TimeT type on Unix and Windows, uses i64 on other targets, and reports the size of TimeT. It adds parsing and implementation for four rounding modes. |
Numeric time conversions crates/vm/src/stdlib/_testinternalcapi/pytime.rs |
The helpers convert integer and float inputs to nanoseconds, time_t values, seconds, and denominator-based fractional values. They check ranges and overflow, reject NaN, and adjust fractional values that cross a denominator boundary. |
Test conversion wrappers crates/vm/src/stdlib/_testinternalcapi.rs |
The wrappers parse positional arguments, reject keywords and incorrect argument counts, and add conversions for seconds, timevals, timespecs, subseconds, and objects. Clamped conversions saturate seconds and set the fractional part to zero. |
Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Possibly related issues
Suggested reviewers: youknowone
Merge Risk: ⚪ Minimal · up to 29404
-
The PyTime conversion helpers have no established merge-blocking defect. Windows and 32-bit execution remain unvalidated, but no specific failure was demonstrated.
Pre-merge checks: 4 · 1
-
Failed checks: 1 warning
| Check name |
Status |
Explanation |
Resolution |
| Docstring Coverage |
 |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 2 files. |
Write docstrings for the functions missing them to satisfy the coverage threshold. |
Passed checks: 4 passed
-
| Check name |
Status |
Explanation |
| Description Check |
 |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
 |
The title clearly and concisely describes the main changes: completing PyTime conversion fixtures and repairing existing helpers. |
| Linked Issues check |
 |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
 |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
-
Comment @coderabbitai help to get the list of available commands.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
youknowdot
pushed a commit
to youknowdot/RustPython
that referenced
this pull request
Oct 22, 2026
Stack the eight consumed conversion APIs on the existing-helper prerequisite in RustPython#9317. Keep the arithmetic local to the real fixture consumers; do not expose _testcapi or change sleep/lifecycle behavior.
Assisted-by: Codex:unknown
youknowdot
changed the title
Fix rounding and argument validation in existing PyTime test helpers
Complete PyTime conversion fixtures and repair existing helpers
Oct 22, 2026
Codex Review Summary
This comment shows the latest Codex review activity on this pull request.
| Review |
Status |
Commit |
Review trigger |
| 📝 Code Review |
✅ Completed 2026-10-24T00:14:16.179443Z |
4aee9e9 |
New commits |
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review" or "@codex security review".
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Extract the already-exposed conversion helper prerequisite from the broader RustPython#9143 surface without adding new test C-API exports or changing time.sleep.
Assisted-by: Codex:unknown
Stack the eight consumed conversion APIs on the existing-helper prerequisite in RustPython#9317. Keep the arithmetic local to the real fixture consumers; do not expose _testcapi or change sleep/lifecycle behavior.
Assisted-by: Codex:unknown
Merging this PR will improve performance by 13.6%
⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks1
Performance Changes
Tip Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing youknowdot:fix-existing-pytime-helpers (4aee9e9) with main (1cd1b5d)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|