Skip to content

Upgrade to Python 3.12.7#5149

Merged
hoodmane merged 16 commits intopyodide:mainfrom
cclauss:python3.12.6
Nov 17, 2024
Merged

Upgrade to Python 3.12.7#5149
hoodmane merged 16 commits intopyodide:mainfrom
cclauss:python3.12.6

Conversation

@cclauss
Copy link
Copy Markdown
Contributor

@cclauss cclauss commented Nov 1, 2024

Description

As discussed here, let's try upgrading from Python 3.12.1 to 3.12.7.

https://github.com/pyodide/pyodide/blob/main/docs/development/maintainers.md#upgrading-pyodide-to-a-new-version-of-cpython
https://github.com/pyodide/pyodide/blob/main/docs/development/maintainers.md#old-major-python-upgrades

-    - image: pyodide/pyodide-env:20240928-chrome127-firefox128
+    - image: pyodide/pyodide-env:20241106-chrome130-firefox132

Issues:

Checklists

@okken

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 6, 2024

I can generate another docker image. But indeed, should it be 3.12.6 or 3.12.7?

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 6, 2024

The test-python tests are failing because this PR fixed the error messages raised by bind(). Nice to see that someone is fixing these.
python/cpython#103404

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 6, 2024

3.12.7 please

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 6, 2024

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 6, 2024

Trying replacing the download URL with https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb.

@cclauss cclauss changed the title DRAFT: Upgrade to Python 3.12.6 DRAFT: Upgrade to Python 3.12.7 Nov 6, 2024
@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 6, 2024

I'm currently pushing pyodide/pyodide-env:20241106-chrome130-firefox132, should be available in a few minutes.

@ryanking13
Copy link
Copy Markdown
Member

ryanking13 commented Nov 7, 2024

The docker image won't build because I'm getting a 404 from
https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/google-chrome-stable_${CHROME_VERSION_FULL}-1_amd64.deb
@ryanking13?

Hmm, does the variable not replaced? I cannot reproduce it locally. Anyway, we'll probably need to add some automated test to test our dockerfile.

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 7, 2024

@cclauss cclauss force-pushed the python3.12.6 branch 2 times, most recently from 09d08b1 to 5f2b45a Compare November 8, 2024 07:30
@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

So it looks like for the core tests we need to skip or xfail leakers.test_ctypes and we need to update the expected error message in test_array_slice_assign_2.

Comment thread src/tests/test_jsproxy.py Outdated
assert exc_info_2a.value.args == exc_info_2b.value.args
assert exc_info_3a.value.args == exc_info_3b.value.args
assert exc_info_3a.value.args == "must assign iterable to extended slice" # type: ignore[comparison-overlap]
assert exc_info_3b.value.args == "can only assign an iterable" # type: ignore[comparison-overlap]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we want to update the code so that the error that the JS Array raises matches the new Python error message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried modifying src/core/jsproxy.c but cannot figure out how to modify exc_info_3a.value.args without also modifying exc_info_3b.value.args.

https://github.com/search?q=repo%3Apyodide%2Fpyodide+%22can+only+assign+an+iterable%22&type=code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure what you mean. The test passes if I apply this change:`

diff --git a/src/core/jsproxy.c b/src/core/jsproxy.c
index 711b15e1..4e3b3d09 100644
--- a/src/core/jsproxy.c
+++ b/src/core/jsproxy.c
@@ -1698,7 +1698,7 @@ JsArray_ass_subscript(PyObject* self, PyObject* item, PyObject* pyvalue)
     slicelength = PySlice_AdjustIndices(length, &start, &stop, step);
 
     if (pyvalue != NULL) {
-      seq = PySequence_Fast(pyvalue, "can only assign an iterable");
+      seq = PySequence_Fast(pyvalue, "must assign iterable to extended slice");
       FAIL_IF_NULL(seq);
     }
     if (pyvalue != NULL && step != 1 &&
diff --git a/src/tests/test_jsproxy.py b/src/tests/test_jsproxy.py
index d8208ecc..26bc88a5 100644
--- a/src/tests/test_jsproxy.py
+++ b/src/tests/test_jsproxy.py
@@ -1520,8 +1520,7 @@ def test_array_slice_assign_2(selenium):
 
     assert exc_info_1a.value.args == exc_info_1b.value.args
     assert exc_info_2a.value.args == exc_info_2b.value.args
-    assert exc_info_3a.value.args == ("must assign iterable to extended slice",)
-    assert exc_info_3b.value.args == ("can only assign an iterable",)
+    assert exc_info_3a.value.args == exc_info_3b.value.args
 
 
 @std_hypothesis_settings

@cclauss cclauss force-pushed the python3.12.6 branch 2 times, most recently from 73035a9 to 5ac054f Compare November 8, 2024 14:01
@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 8, 2024

How could so much change between Python 3.12.1 and 3.12.7?
How could testing the abstract_base_classes module fail?
How can I run these tests on localhost?
Is there some other way of quickly discovering the failing tests?
Should we be waiting for a release that fixes SeleniumHQ/selenium#14699 or is the downgraded Selenium OK to use?
I tried running python src/tests/make_test_list.py locally but that resulted in an empty list.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

I'll look into failing tests. You should be able to run them locally with:

python3.12 -m venv .venv
source .venv/bin/activate
pip install -r requirements.txt
make
pytest src/tests/test_core_python.py

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

Are you getting:

Dynamic linking error: cannot resolve symbol _PyTestCapi_Init_Run

it's a bit perplexing I'll look into it.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

Looks like python/cpython#117982 caused the trouble. We probably need to update packages/test/meta.yaml.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

Okay the core Python tests all pass when I run locally now. Fingers crossed they work in CI too.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

Opened a PR to fix make_test_list.py :
#5177

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

I'm okay with using the downgraded selenium for now. @ryanking13 you have any objection?

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 8, 2024

It was the make step that I missed... Kinda important! Thx.

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 8, 2024

https://pyodide.org/en/stable/console.html vs.
https://pyodide.org/en/latest/console.html

Do you distinguish between stable and latest anymore? If so, (assuming the tests pass) would it make sense to have 3.12.7 be latest while stable remains at 3.12.1 for a week or two while we wait for the Selenium release? That way a some folks can kick the tires before we switch stable.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

latest is perhaps misnamed, it's actually unstable tip of tree. I imagine we'll just release this as part of Pyodide 0.27.0.

@hoodmane
Copy link
Copy Markdown
Member

hoodmane commented Nov 8, 2024

Okay core test suite looking good. @ryanking13 can you look at the pygame-ce build error?

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 8, 2024

ci/circleci: build-packages-no-numpy-dependents — Tests failed on CircleCI on pygame-ce.

pygame-ce-2.4.1

url: https://files.pythonhosted.org/packages/dc/b1/64fffc2c8664497ae82b2afb4f5efe0130d38b39f2d25af0288c4261df3e/pygame-ce-2.4.1.tar.gz

vs. pygame-ce-2.5.2 at https://pypi.org/project/pygame-ce however this repo contains a few patches for pygame-ce.

https://github.com/search?q=repo%3Apyodide%2Fpyodide%20pygame-ce

@ryanking13
Copy link
Copy Markdown
Member

I'm okay with using the downgraded selenium for now. @ryanking13 you have any objection?

No objection.

@ryanking13
Copy link
Copy Markdown
Member

ryanking13 commented Nov 9, 2024

Okay core test suite looking good. @ryanking13 can you look at the pygame-ce build error?

I am not sure about the build error, but it seems like there was some change in the problematic files in recent pygame-ce. So let me try updating the pygame-ce version and see what happens.

@hoodmane
Copy link
Copy Markdown
Member

Can you add a changelog entry? Other than that looks good to go if CI passes.

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 17, 2024

Dedicated to the great work of @okken

As he implied at https://www.youtube.com/watch?v=EB2emah5d5g&t=9m40s Python 3.12.1 is a bit out of date ;-)

@hoodmane
Copy link
Copy Markdown
Member

We try to stay on a version that is at most 18 months old =)

@cclauss cclauss mentioned this pull request Nov 17, 2024
3 tasks
@hoodmane hoodmane added this to the 0.27.0 milestone Nov 17, 2024
@hoodmane
Copy link
Copy Markdown
Member

Thanks @cclauss!

@hoodmane hoodmane merged commit e37e54c into pyodide:main Nov 17, 2024
@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 17, 2024

Was an update to pyodide-cross-build-environments.json required?!?

@cclauss cclauss deleted the python3.12.6 branch November 17, 2024 12:55
@hoodmane
Copy link
Copy Markdown
Member

No, only after Pyodide is released.

@cclauss
Copy link
Copy Markdown
Contributor Author

cclauss commented Nov 18, 2024

Great! https://pyodide.org/en/latest/console.html is now 3.12.7.

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