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

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

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

M5oul
Copy link
Contributor

@M5oul M5oul commented Dec 4, 2024

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
Copy link
Collaborator

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

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

src/pendulum/utils/_zoneinfo.py Outdated Show resolved Hide resolved
Copy link

codspeed-hq bot commented Dec 4, 2024

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
Copy link
Collaborator

Secrus commented Dec 5, 2024

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 Secrus requested a review from sdispater December 5, 2024 12:14
@M5oul
Copy link
Contributor Author

M5oul commented Dec 5, 2024

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
Copy link
Contributor Author

M5oul commented Dec 5, 2024

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
Copy link
Collaborator

Secrus commented Dec 6, 2024

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

@Minku-Koo
Copy link
Contributor

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

@Secrus
Copy link
Collaborator

Secrus commented Dec 17, 2024

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 Secrus merged commit 483443a into python-pendulum:master Dec 17, 2024
17 of 18 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.

4 participants