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

Update to Python 3.9.5 #1637

Merged
merged 76 commits into from
Jun 19, 2021
Merged

Update to Python 3.9.5 #1637

merged 76 commits into from
Jun 19, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jun 14, 2021

Closes #978

@rth
Copy link
Member

rth commented Jun 14, 2021

Thanks for working on this @hoodmane !

@hoodmane
Copy link
Member Author

A lot of packages are failing to build with the error:

[wasm-validator error in module] unexpected true: Imported global cannot be mutable,

This comes from wasm-opt. If I remove --mvp-features and add --enable-mutable-globals the error goes away. I guess I'll try patching emscripten with this...

src/js/pyodide.js Outdated Show resolved Hide resolved
# further investigation. This usually seems to be caused by calling into a
# system function that doesn't behave as one would expect.
# - crash-chrome: Same as crash but only affecting Chrome
# - crash-firefox: Same as crash but only affecting Firefox
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have this list either in that file or here, not both. Otherwise it's going to be annoying to maintain in sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it so that make_test_list.py writes this header into python_tests.txt. python_tests.txt is a weird mix of a generated file and a hand modified file...

src/tests/make_test_list.py Outdated Show resolved Hide resolved
src/tests/test_pyodide.py Show resolved Hide resolved
Patch building.py to deal with '[wasm-validator error in module] unexpected true: Imported global cannot be mutable'
@rth
Copy link
Member

rth commented Jun 17, 2021

Were the issues at build time or at run time?

If I revert scikit-learn, scikit-image, statsmodels setup to the version on main, and then change the URL to a GitHub tag 3e28f4a they all build fine for me locally as part of this PR.

  • scikit-image tests also pass
  • scikit-learn and statsmodels fail to import which need more investigation.

@hoodmane
Copy link
Member Author

hoodmane commented Jun 17, 2021

At build time. I haven't tested whether they build locally until just recently, I was just looking at the CI. My modification to regenerate the statsmodels patches fixed the statsmodels build in CI. They fail with

error: no member named 'tp_print' in 'struct _typeobject'

in Cython-generated files. See here for example:

https://app.circleci.com/pipelines/github/hoodmane/pyodide/718/workflows/32426aa7-7c1c-4799-9d82-8b51f5bc152f/jobs/5937

@hoodmane
Copy link
Member Author

If I revert scikit-learn, scikit-image, statsmodels setup to the version on main, and then change the URL to a GitHub tag 3e28f4a they all build fine for me locally as part of this PR.

Could you push this here?

@hoodmane
Copy link
Member Author

On CI just now the Firefox statsmodels import test passes. Did you try to import it in Chrome or Firefox? We marked that test xfail in Chrome.

@rth
Copy link
Member

rth commented Jun 18, 2021

Tests for statsmodels and scikit-image also passed when installed from GH with no other changes, but only after restarting CI. So I wonder if it means there is a function signature mismatch somewhere and it fails half the times.

@hoodmane
Copy link
Member Author

there is a function signature mismatch somewhere and it fails half the times.

Is that a characteristic symptom of function signature mismatch? It does seem particularly flaky here.

@rth
Copy link
Member

rth commented Jun 18, 2021

Well one of possible causes

# linker and C flags are set to error on anything to do with function declarations being wrong. By default these
# are just warnings, but in webassembbly, any conflicts mean that a randomly selected 50% of calls to the function
# will fail. This is understandably bad. Better to fail at compile or link time

Actually maybe we should apply those flags globally to all packages.

@hoodmane
Copy link
Member Author

Actually maybe we should apply those flags globally to all packages.

Sounds logical to me.

@hoodmane
Copy link
Member Author

a randomly selected 50% of calls will fail

Is the use of the word "random" here accurate? Or does it really mean that some consistent fraction of the calls will fail? I don't really understand the mechanism that would cause the randomness here. Seems to me that there is a chance we are just hitting on typical CI flakiness here.

@hoodmane
Copy link
Member Author

Okay all tests passed. I think we should proceed with merging this.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work @hoodmane !

A few comments otherwise LGTM!

Please also,

  • add a changelog entry
  • don't forget to push the docker image and update .circleci/config.yml

src/tests/make_test_list.py Outdated Show resolved Hide resolved
pyodide-build/pyodide_build/buildpkg.py Outdated Show resolved Hide resolved
@rth
Copy link
Member

rth commented Jun 19, 2021

Is the use of the word "random" here accurate? Or does it really mean that some consistent fraction of the calls will fail? I don't really understand the mechanism that would cause the randomness here. Seems to me that there is a chance we are just hitting on typical CI flakiness here.

I think such issues might explain in part the CI flakiness. It does look like under some conditions emcripten compiled code fails non deterministically, there is some number of such issues in their issue tracker. Also apparently it's a documented behavior for va_arg,

If there is no next argument, or if type is not compatible with the type of the actual
next argument (as promoted according to the default argument promotions), random errors will occur. 

@hoodmane
Copy link
Member Author

under some conditions emscripten compiled code fails non deterministically

This is certainly good to know. Sad though...

random errors will occur

Right... when I read these things I usually plug my ears and scream "unpredictable but repeatable". But one thing UB is allowed to do is fail non repeatably. In the va_arg example, presumably it depends what is in the memory on the stack next to the last argument, which will vary from run to run based on who-knows-what.

@hoodmane hoodmane mentioned this pull request Jun 19, 2021
@hoodmane hoodmane merged commit b4f4bcf into pyodide:main Jun 19, 2021
@hoodmane hoodmane deleted the python-3.9 branch June 19, 2021 22:49
hamlet4401 pushed a commit to tytgatlieven/pyodide that referenced this pull request Jul 3, 2021
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
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.

Update to python 3.9.1
2 participants