-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fixes for compatibility with Python 3.13 #1321
base: unstable
Are you sure you want to change the base?
Conversation
| 0.838545 0.247025 | ||
| 0.838697 0.436220 | ||
| 0.309496 0.833591 | ||
| 0.629452 0.586355 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In docstrings, use 4 spaces instead of a tab character.
.github/workflows/ubuntu.yml
Outdated
@@ -11,7 +11,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python-version: ['3.12', '3.11', '3.8', '3.9', '3.10'] | |||
python-version: ['3.13', '3.12', '3.11', '3.8', '3.9', '3.8'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should keep 3.8? Maybe 3.8, 3.10, 3.12 and 3.13 would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am for dropping 3.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so ['3.13', '3.12', '3.11', '3.10', '3.9'] in all the workflows?
@@ -468,6 +468,8 @@ def get_functions(self, prefix="eval", is_pymodule=False): | |||
for name in dir(self): | |||
if name.startswith(prefix): | |||
function = getattr(self, name) | |||
if not hasattr(function, "__call__"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, NoneType
has a docstring, so we need to check that function
is indeed a function.
@@ -93,7 +93,7 @@ | |||
r"""(?mx)^ # re.MULTILINE (multi-line match) | |||
# and re.VERBOSE (readable regular expressions | |||
((?:.|\n)*?) | |||
^\s+([>#SX])>[ ](.*) # test-code indicator | |||
^\s*([>#SX])>[ ](.*) # test-code indicator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Python 3.13, doctrings are trimmed of "margin" spaces.
|
||
# Remove leading <dl>...</dl> | ||
# doc = DL_RE.sub("", doc) | ||
doc = filter_comments(doc) # .strip("\s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not remember the reason of the r"\s" strip, but it breaks some tests now.
@@ -394,11 +391,16 @@ def compare_out(self, outs: tuple = tuple()) -> bool: | |||
# Mismatched number of output lines, and we don't have "..." | |||
return False | |||
|
|||
# Python 3.13 replaces tabs by a single space in docstrings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python 3.13 converts tab characters in docstrings to simple spaces. This "hack" helps to keep the doctests compatible with this change.
Note: Supporting 3.13 may push us into using a newer pip that disallows the Keep in mind, getting everything working on 3.13 where we can release might be a bigger undertaking than just getting the Mathics Kernel adjusted. |
OK, this is just to adjust the kernel compatibility, and does not breaks the compatibility with previous versions. |
And that means we cannot do a full release like we just did until everything is fixed. |
.github/workflows/ubuntu.yml
Outdated
@@ -11,7 +11,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python-version: ['3.13', '3.12', '3.11', '3.8', '3.9', '3.8'] | |||
python-version: ['3.13', '3.12', '3.10', '3.8'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to change 3.8 to 3.9?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was saying to keep the even versions, plus the last one (if odd).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.8 is the version supported by the last version of Pyston.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should drop Pyston then in its standalone form. There is still a pyston-lite plugin.
I don't see a reason to drop versions that end in an odd number or not test them.
Python does not say that minor numbers that end in odd numbers have any less status than those with even numbers. Dropping 3.8 makes sense because the Python language drifts and it is extra effort to support older Pythons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a pitty because Pyston is much more faster than CPython and Pyston-lite, but OK. In any case, we still are compatible with 3.8
9e2a614
to
f73364b
Compare
I don't have a problem with the idea or the code, but the timing of doing this right now is weird. I've just spent the last 3 days if not longer trying to get out a release ensuring all the parts work together. (And I am not sure they completely do). Something like this now ensures things will be a little bit out of whack again and puts us back in the state of a little off-ness. I don't think some of the code will be able to handle 3.13 unless an older pip is used. And code in other repositories may need additional adjusting too. And of course, there are other PRs in the backlog. These change the API which will also require changing some other bits of code outside of this repository. To accommodate moving forward which we need to do and keep some stability for a little while right now, I suggest we have an "unstable" branch that we merge to, along with the master branch which is compatible across other projects. Your thoughts? |
Hmm. We could split this up into two parts, isolate that version testing change part and everything else. So that when we are ready to switch over everything there is a small bit of code in this repository that does that. |
As with much of the other stuff I did, the timing depends more on finding the time to do it than on achieving a deadline or a particular general goal.
The changes here (except for the tests) ensure that mathics-core tests still pass in Python 3.13. This is not a release or something that breaks the rest of the code. The changes do not seem to affect the behavior of the rest of the components, but in six months or a year, when we want to start the next release, they would make the process simpler.
That sounds reasonable.
|
This is a problem that can be solved by working in a more coordinated way. From the project's perspective, it is better to have a plan for moving forward that we agree on and discuss. You put in a checklist of ideas for the 8.0.0 release. Some of these have been deferred. Items in that list were picked precisely not to be disruptive. And we have open issues. I suppose we should add upgrading to 3.13 to that list, but 3.13 was not on that list. Upgrading to a newer Python release, especially with the way Python and its ecosystem move nowadays, is disruptive. And if you have a question about doing something you could just mention it or ask about it in advance. |
OK, but the changes in this PR are closely related to the work done before the release, relative to adjusting the BTW, the support for LaTeX expressions in the online Mathics-Django using MathJax, something that was in the list for the pre-release, is done: Mathics3/mathics-django#221 |
You may see it that way, but as the discussion I hope shows, it isn't like that. There could be a lesson here: mention first, code later.
Again, it is not about whether certain changes are good, but more about coordinating, timing, and appropriateness for the situation right now.
Thank you!
Sorry, I lost track of this. In my last look, you mentioned that MathJaX needed to be updated. I see that's been done now. Looking at it and trying now. Thanks for doing this. As for release, We can always come out with a minor update for the Django portion. And that might be a good thing to do. The Django portion has this weirdness that we are not using PyPI for CI testing. It might have something to do with where it is expecting JSON tables to be located or that the tables are not getting created. I let this slide in the overall 8.0.0 revision of everything because things were just getting delayed. |
This PR contains the fixed needed to make Mathics compatible with Python 3.13.