Python3 migration & Emscripten update #133
Conversation
…hoose between two options in PR
|
@albincorreya let me know what you think about the 2 notes in question, please |
Dockerfile
Outdated
| && rm /tmp/requirements.txt | ||
| # install essentia python bindings from pre-compiled wheels | ||
| COPY src/python/essentia-2.1b6.dev861-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl /tmp/essentia-2.1b6.dev861-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl |
There was a problem hiding this comment.
Why not directly install the pip wheels from PyPi? Ideally we shouldn't store binaries in the repository.
There was a problem hiding this comment.
Ideally we shouldn't need to install this if we compile essentia with python3 bindings. It will install the latest python library as well. I don't remember the details but there was some issues with it at that time. We could test it again to see if it solves now.
There was a problem hiding this comment.
I seem to remember I found some issues back in september when I did this, but let me re-try with a regular PyPi install to see if it works
There was a problem hiding this comment.
you're right, this still produces a Could not build python extensions error, so I'll leave essentia in requirements.txt for now
| NPM_PATH=/emsdk/node/12.18.1_64bit/bin/npm | ||
| NODE_PATH=/emsdk/node/12.18.1_64bit/bin/node | ||
| # NPM_PATH=/emsdk/node/12.18.1_64bit/bin/npm | ||
| # NODE_PATH=/emsdk/node/12.18.1_64bit/bin/node |
There was a problem hiding this comment.
we can remove these comments if the entrypoint issues are resolved
There was a problem hiding this comment.
yes, I didn't find any issues with the latest emscripten base docker image
There was a problem hiding this comment.
However I can see the CI build workflow failing because it grabs the pre-built docker image from Docker Hub, so the change to the newer emscripten version is not reflected on the CI workflow.
I wonder if there is a way to build the image from the Dockerfile on the github CI workflow for these changes to the build pipeline instead of grabbing from Docker Hub...
There was a problem hiding this comment.
It's definitely possible to build the Docker image in a GitHub workflow, but I should create a new build-dev workflow for when we commit changes to the Dockerfile itself and the other build scripts.
I can do that on this PR to ensure that the other changes result in a successful build process.
There was a problem hiding this comment.
I have done this now and checks are passing. I would remove the files being watched on the new "build-dev" workflow from the other two workflows (integration.yml and deploy.yml), let me know what you think about that @albincorreya
There was a problem hiding this comment.
Great job! We could remove these comments in build-libs.sh since it is not needed anymore?
| @@ -1,4 +1,6 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I assume we will migrate these old python2 scripts to python3 after fixing the build pipeline?
There was a problem hiding this comment.
as above, my intention is to do it on this PR
I think all scripts now are compatible with Python 3, but let me know if you see anything that should clearly be changed
There was a problem hiding this comment.
Looks like code_generator.py script has python2 string formatting, not sure if python3 supports that? maybe I missed something here
There was a problem hiding this comment.
@albincorreya I see what you mean. It seems that modulo string formatting still works in Python3, although f-strings are the preferred syntax. If it's okay with you, I think it makes sense to do the switch to f-strings on the code_generator.py script on the object-oriented PR that I'm working on, because that will already change significant portions of the script.
There was a problem hiding this comment.
@jmarcosfer make sense, let's merge this then 💯
I don't see any noticeable performance difference between the two methods, so I'll stick to the newer default way, which also looks cleaner as you say :) |
| - dev | ||
| - master | ||
| paths: | ||
| - 'src/python/requirements.txt' |
| NPM_PATH=/emsdk/node/12.18.1_64bit/bin/npm | ||
| NODE_PATH=/emsdk/node/12.18.1_64bit/bin/node | ||
| # NPM_PATH=/emsdk/node/12.18.1_64bit/bin/npm | ||
| # NODE_PATH=/emsdk/node/12.18.1_64bit/bin/node |
There was a problem hiding this comment.
Great job! We could remove these comments in build-libs.sh since it is not needed anymore?
| @@ -1,4 +1,6 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Looks like code_generator.py script has python2 string formatting, not sure if python3 supports that? maybe I missed something here
Two main changes to the build pipeline before other development tasks:
Notes:
-c,-r, or-sharedflag is given. Therefore commands have changed slightly and we have two options: either (1) build to object files first, like we used to do, but individually forbindings_essentiajs.oandessentiajs.obecause now it doesn't accept multiple files with-oflag; (2) use the new default way, which also means fewer commands, and go directly from C++ to JS+WASM.