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

Testing against Python 3.12 (non-dev) and Python 3.13 (dev) #1079

Merged
merged 6 commits into from Feb 17, 2024

Conversation

etianen
Copy link
Contributor

@etianen etianen commented Feb 10, 2024

This PR adds Python 3.13 (dev) to the test matrix, and changes the Python 3.12 (dev) entry to 3.12 (non-dev).

Python 3.7 is now EOL, so might be worth dropping from the test matrix. I'll happily provide a follow-up PR if you're interested.

@etianen etianen marked this pull request as draft February 10, 2024 14:39
@etianen
Copy link
Contributor Author

etianen commented Feb 10, 2024

That is... an astonishing number of errors.

The pickling errors I can trace back to python/cpython#109462. It looks like setting self.lock = None is not gonna fly with Python 3.13.

A lot of the other errors seem to be due to a difference in traceback formatting. An obvious fix is to loosen up a lot of the result.count calls, but I'm not sure how important those are.

@etianen
Copy link
Contributor Author

etianen commented Feb 10, 2024

I'm happy to work through these fixes. Since this is non-trivial now, and will take a bit of time, is it something you'd like me to continue with?

@Delgan
Copy link
Owner

Delgan commented Feb 11, 2024

Thanks for the updates!

Regarding Python 3.7 being EOL, I still like to keep testing and maintaining this version (with Python 3.5 and 3.6 as well, by the way). It's not much of a burden, so I prefer not to break compatibility with Python versions before a possible 1.0.0 release of Loguru.

Sorry about the difficulties you're facing with Python 3.13 and thank you for the preliminary findings. It indeed seems non-trivial. However, I think we should wait for the "beta" version of Python 3.13 before testing it in the CI. Since it's still in "alpha" (see release schedule), breaking changes may be introduced daily, and I don't want this to disrupt Loguru's CI.
However, perhaps we can add the job and allowing it to fail, to have a hint as soon as possible that a fix will be needed?

@etianen
Copy link
Contributor Author

etianen commented Feb 11, 2024

That all sounds very sensible, both with supporting older versions and avoiding CI disruption for newer ones.

I'll have a go at fixing this up for Python 3.13, but keep the CI job fail-able. I've got pretty limited time for open source these days, but I get the feeling there's a few small underlying tweaks needed and this will all fall into place. I'll keep the PR draft until then. 🖖

@Delgan
Copy link
Owner

Delgan commented Feb 11, 2024

Don't feel compelled to do this, but thanks for your help!

The `Handler.lock` is substituted for a `nullcontext`, keeping Python 3.13 happy. A no-op stub for `Handler.release` is added, keeping all other Python versions happy.
The `Handler.lock` is substituted for a `MockLock`, keeping Python 3.13 happy. A no-op stub for `Handler.release` is added, keeping all other Python versions happy.
This is fixed with a forward-port of `traceback.format_list` from Python 3.5.
@etianen
Copy link
Contributor Author

etianen commented Feb 12, 2024

The test_pickling errors were easily solved. 😅

The errors with traceback formatting were... not easily solved 💩. Python 3.13 has thrown a real spanner in the works here.

Specifically, in Python 3.13, unless you provide a colno and end_colno to the FrameSummary, it only prints the first line of the traceback source, completely annihilating your nice variable annotations (https://github.com/python/cpython/blob/main/Lib/traceback.py#L571).

This throws up some real problems:

  • colno and end_colno don't even exist in older supported Python versions!
  • If you supply them to more recent Python versions, it farts ^^^^^^^^^^^^^^^^^^^^^^ characters all over the traceback formatting.
  • If you supply them to Python 3.13, it makes some other unfathomable assumptions about the contents of FrameSummary.line and prints nothing whatsoever.

This leads me to a horrible conclusion - traceback.format_list() is not suitable for formatting your nice traceback annotations in Python 3.13. 😱

My solution is to take the original format_lines implementation from Python 3.5, which is actually really simple, and include it (slightly modified) in _better_exceptions.py. The tracebacks now look nice in all Python versions!

Upsides:

  • It works!
  • We're no longer tied to the eccentricities of Python stack formatting.

Downsides:

  • A few more LOC to maintain.
  • No support for some of the improvements to stack formatting in later Pythons (e.g. "line repeated N times" in Python 3.7 for highly-recursive tracebacks).

We could mitigate this last downside by forward-porting a format_lines implementation from, say, Python 3.7 rather than Python 3.5. The implementation gets more complex with each Python version, of course. This is your call, naturally. The Python 3.7 implementation might be a better balance between simplicity and features.

Or maybe you know something about the internals of traceback formatting that I don't! This is the best I've got, in any case! 🙇

@etianen etianen marked this pull request as ready for review February 12, 2024 01:03
@etianen
Copy link
Contributor Author

etianen commented Feb 12, 2024

On reflection, I think there's another benefit to taking fully control of traceback formatting. We're already performing at least two passes over this traceback to format it, then colorise it. Although this is a quick way to fix the Python 3.13 problem, it opens up doors to significantly optimize the customized traceback formatting into a nice single-pass efficient render.

If you're okay with this approach, I'd like to to forward-port the Python 3.7 traceback formatting to get the recursion handling. But then I think this a good future-proof solution that paves the way for improving and optimizing traceback rendering without the shackles of the stdlib.

@Delgan Delgan merged commit 9311c76 into Delgan:master Feb 17, 2024
17 checks passed
@Delgan
Copy link
Owner

Delgan commented Feb 17, 2024

That looks amazing! These are great findings and fixes, thanks a lot for the efforts you've put into this.

The custom implementation of format_list() is very straightforward, it's not a problem to maintain it. As this reduces our dependence on traceback, it also makes formatting more resilient to unexpected changes as you said.

Regarding the additional features brought by Python 3.7, I guess we can argue they are not "officially" supported by Loguru since there is no unit tests for them. However, I expect users to complain about regression if Loguru no longer detects repeated frames in traceback. It would be nice to have it, I created #1083.

On a side note, Loguru's formatting was originally designed to match the built-in formatting perfectly, with the addition of colors and variables. Today, this goal is no longer possible. It seems difficult to reconcile the built-in's ~~~^~ underlining, with Loguru's arrows positioning.

@etianen
Copy link
Contributor Author

etianen commented Feb 17, 2024

Nice! Thanks!

Shall I forward-port the Python 3.7 traceback and add the unit tests to close #1083?

@Delgan
Copy link
Owner

Delgan commented Feb 17, 2024

If you're interested in implementing this improvement, your contribution will be very welcome once again. Note that unit tests for exception formatting are generated by uncommenting this line.

I mainly created the ticket to keep track of what needs to be done, and to do it eventually before the next release. ;)

@etianen etianen deleted the drop-3.7 branch February 17, 2024 17:27
@etianen
Copy link
Contributor Author

etianen commented Feb 17, 2024

Actually... hrm... although I'd actually love to, my parenting duties are cutting severely into my open source time! So I'm gonna do the sensible dad thing and leave this one to you. 😢

@Delgan
Copy link
Owner

Delgan commented Feb 17, 2024

Haha, that's definitely the right decision to make. 😄

Thanks again for your work!

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.

None yet

2 participants