Skip to content

Complete PyTime conversion fixtures and repair existing helpers - #9317

Merged
youknowone merged 2 commits into
RustPython:mainfrom
youknowdot:fix-existing-pytime-helpers
Oct 24, 2026
Merged

youknowone merged 2 commits into
RustPython:mainfrom
youknowdot:fix-existing-pytime-helpers

Conversation

@youknowdot

@youknowdot youknowdot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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:

  • 🔍 Trigger review

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: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning 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 Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main changes: completing PyTime conversion fixtures and repairing existing helpers.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed 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)
  • Create a new PR
  • 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.

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 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
@youknowone
youknowone marked this pull request as ready for review October 23, 2026 08:57
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

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.

youknowdot and others added 2 commits October 24, 2026 08:14
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
@youknowdot
youknowdot force-pushed the fix-existing-pytime-helpers branch from 2940485 to 4aee9e9 Compare October 24, 2026 00:01
@codspeed

codspeed Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will improve performance by 13.6%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ rustpython[comprehension_list.py] 907.4 µs 798.8 µs +13.6%

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)

Open in CodSpeed

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@youknowone
youknowone merged commit 413710c into RustPython:main Oct 24, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants