FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Drop Py3.8 support to add potential Py3.13 support by M5oul · Pull Request #863 · python-pendulum/pendulum · GitHub

Drop Py3.8 support to add potential Py3.13 support - #863

Merged
Secrus merged 5 commits into
python-pendulum:masterfrom
M5oul:master
Dec 17, 2024
Merged

Drop Py3.8 support to add potential Py3.13 support#863
Secrus merged 5 commits into
python-pendulum:masterfrom
M5oul:master

Conversation

M5oul commented Dec 4, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

at least from the dependencies point of view.
Bumping dependencies, which dropped Py3.8 supports, also adds Py3.13 support.

(#844)

Officially EOL by Python project

Run `poetry update`

Drop "backports.zoneinfo", importlib-resources

Bump time-machine, pytest-codspeed to last version which support Py3.13
https://github.com/adamchainz/time-machine/blob/main/CHANGELOG.rst
https://github.com/CodSpeedHQ/pytest-codspeed/releases

Secrus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One issue for now, let's see what CI says

Comment thread src/pendulum/utils/_zoneinfo.py Outdated

codspeed-hq Bot commented Dec 4, 2024
edited
Loading

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #863 will degrade performances by 91.71%

Comparing M5oul:master (73cfa18) with master (09d815a)

🎉 Hooray! pytest-codspeed just leveled up to 3.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

❌ 1 regressions

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master M5oul:master Change
test_parse_iso8601 1.2 ms 14.3 ms -91.71%

Secrus commented Dec 5, 2024

Copy link
Copy Markdown
Collaborator

I am no expert on Rust/PyO3, but are the changes to the code needed? There is a huge performance regression. It's unmergable in that state.

Secrus requested a review from sdispater December 5, 2024 12:14

M5oul commented Dec 5, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

I cherry-picked last commit from https://github.com/dlt-hub/pendulum/, otherwise, we get rust-lint errors that can be found bellow. I can’t say if the Rust changes are any good.

error: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> src/python/types/duration.rs:23:1
   |
23 | #[pymethods]
   | -^^^^^^^^^^^
   | |
   | `PyClassImplCollector` is not local
   | `PyClassNewTextSignature` is not local
   | move the `impl` block outside of this function `trampoline` and up 4 bodies
24 | impl Duration {
   |      --------
   |      |
   |      `Duration` is not local
   |      `Duration` is not local
   |
   = note: the attribute macro `pymethods` defines the non-local `impl`, and may need to be changed
   = note: the attribute macro `pymethods` may come from an old version of the `pyo3_macros` crate, try updating your dependency with `cargo update -p pyo3_macros`
   = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
   = note: `-D non-local-definitions` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(non_local_definitions)]`
   = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

error: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> src/python/types/precise_diff.rs:23:1
   |
23 | #[pymethods]
   | -^^^^^^^^^^^
   | |
   | `PyClassImplCollector` is not local
   | `PyClassNewTextSignature` is not local
   | move the `impl` block outside of this function `trampoline` and up 4 bodies
24 | impl PreciseDiff {
   |      -----------
   |      |
   |      `PreciseDiff` is not local
   |      `PreciseDiff` is not local
   |
   = note: the attribute macro `pymethods` defines the non-local `impl`, and may need to be changed
   = note: the attribute macro `pymethods` may come from an old version of the `pyo3_macros` crate, try updating your dependency with `cargo update -p pyo3_macros`
   = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
   = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

error: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> src/python/types/timezone.rs:11:1
   |
11 | #[pymethods]
   | -^^^^^^^^^^^
   | |
   | `PyClassImplCollector` is not local
   | `PyClassNewTextSignature` is not local
   | move the `impl` block outside of this function `trampoline` and up 4 bodies
12 | impl FixedTimezone {
   |      -------------
   |      |
   |      `FixedTimezone` is not local
   |      `FixedTimezone` is not local
   |
   = note: the attribute macro `pymethods` defines the non-local `impl`, and may need to be changed
   = note: the attribute macro `pymethods` may come from an old version of the `pyo3_macros` crate, try updating your dependency with `cargo update -p pyo3_macros`
   = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
   = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `_pendulum` (lib test) due to 3 previous errors
make: *** [Makefile:13: lint-rust] Error 101

codspeed performances regression seems related to its version bump, not the Pyo3/rust commit. I don’t have a significant difference with or without this commit:

┏━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━┓
┃          Benchmark ┃ Time (best) ┃ Rel. StdDev ┃ Run time ┃  Iters ┃ with commit
┡━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━┩
│ test_parse_iso8601 │   103,137ns │        3.1% │    2.91s │ 26,072 │
└────────────────────┴─────────────┴─────────────┴──────────┴────────┘
┏━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━┓
┃          Benchmark ┃ Time (best) ┃ Rel. StdDev ┃ Run time ┃  Iters ┃ without commit
┡━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━┩
│ test_parse_iso8601 │   101,767ns │        5.0% │    3.00s │ 27,215 │
└────────────────────┴─────────────┴─────────────┴──────────┴────────┘

M5oul commented Dec 5, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

As said in codspeed message:

🎉 Hooray! pytest-codspeed just leveled up to 3.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

I gave it a try to compare on Py3.12, with only bumping pytest-codspeed version, based on master branch:

  • v1.2.2: == 1 passed, 1825 deselected in 0.24s
  • v2.2.1: == 1 passed, 1825 deselected in 0.24s
  • v3.0.0: == 1 passed, 1825 deselected in 4.28s:
┏━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━┓
┃          Benchmark ┃ Time (best) ┃ Rel. StdDev ┃ Run time ┃  Iters ┃
┡━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━┩
│ test_parse_iso8601 │   147,445ns │        4.4% │    3.00s │ 18,919 │
└────────────────────┴─────────────┴─────────────┴──────────┴────────┘

I don’t have this table with previous versions, not sure which value should be taken.

Secrus commented Dec 6, 2024

Copy link
Copy Markdown
Collaborator

I will try to find some time to test the changes myself. I just want to be extra careful and double-check everything.

Copy link
Copy Markdown
Contributor

@M5oul How can I solve problem about rust impl?
can you share me commit id?

Secrus commented Dec 17, 2024

Copy link
Copy Markdown
Collaborator

Ok, I have tested the changes locally and it seems to be alright, gonna merge it and see how it behaves with some PRs I plan on merging soon-ish.

Secrus merged commit 483443a into python-pendulum:master Dec 17, 2024
ashb mentioned this pull request Jan 13, 2025
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL