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

Python updates #86

Merged
merged 20 commits into from Dec 2, 2019
Merged

Python updates #86

merged 20 commits into from Dec 2, 2019

Conversation

@farizrahman4u
Copy link

farizrahman4u commented Nov 27, 2019

No description provided.

farizrahman4u and others added 14 commits Oct 29, 2019
Signed-off-by: Alex Black <blacka101@gmail.com>
@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Nov 27, 2019

Confirmed passing all datavec tests on linux

On Windows I'm getting this...
https://gist.github.com/AlexDBlack/68fe556289fcd32a16b31304a3218a17

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Nov 28, 2019

So, there's a legit issue here on Windows

If I delete javacpp cache and run: OK, passes
If I then run again: it fails as per the gist in my previous post

Somehow it's actually modifying the cache files?
here's C:\Users\Alex\.javacpp\cache\numpy-1.17.3-1.5.2-windows-x86_64.jar\org\bytedeco after the first run (which succeeds):
image

And here it is after the second run (without deleting the javacpp cache between runs; it fails):
image

Note the slightly different total file size - 16,550,850 vs. 16,550,691
Same number of files/folders though, so something is getting modified?

cc @saudet

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Nov 28, 2019

Can you run a diff -ru on that and see which files are changed?

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Nov 28, 2019

After running once, renaming cache to cache_singlerun, then running twice (on same default cache directory, with one pass and one failure as before):

diff --brief --recursive /mnt/c/Users/Alex/.javacpp/cache/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy /mnt/c/Users/Alex/.javacpp/cache_singlerun/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy

(note run via WSL 2 for diff tool)

I get the following:

Files /mnt/c/Users/Alex/.javacpp/cache/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy/windows-x86_64/python/numpy/core/__pycache__/overrides.cpython-37.pyc and /mnt/c/Users/Alex/.javacpp/cache_singlerun/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy/windows-x86_64/python/numpy/core/__pycache__/overrides.cpython-37.pyc differ
Files /mnt/c/Users/Alex/.javacpp/cache/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy/windows-x86_64/python/numpy/testing/_private/__pycache__/utils.cpython-37.pyc and /mnt/c/Users/Alex/.javacpp/cache_singlerun/numpy-1.17.3-1.5.2-windows-x86_64.jar/org/bytedeco/numpy/windows-x86_64/python/numpy/testing/_private/__pycache__/utils.cpython-37.pyc differ
@saudet

This comment has been minimized.

Copy link
Member

saudet commented Nov 28, 2019

Looks like it's Python corrupting its own cache here, nothing to do with JavaCPP. CPython isn't thread-safe by default, so it's probably us trying to use it from multiple threads without the proper locks: https://docs.python.org/3/c-api/init.html

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Nov 28, 2019

There shouldn't be any multithreading here afaik, unless it's done internally in python executor... @farizrahman4u ?
Edit: also I didn't see any issues on linux
So maybe some sort of RC, between shutdown of one test (python executor) and startup of the next

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Nov 28, 2019

Or something not properly closed between tests - @farizrahman4u ?
Unfortunately python isn't exactly my specialty, not sure how useful I'll be in debugging this at this point...

@farizrahman4u

This comment has been minimized.

Copy link
Author

farizrahman4u commented Dec 1, 2019

@AlexDBlack I dont think it was windows vs linux.. its different versions of numpy (you probably have different versions of numpy on windows and linux(wsl)). Should be fixed now, delete javacpp python cache and try again.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Dec 1, 2019

So this is a problem with NumPy? I'd recommend using the version distributed with the presets instead of depending on whatever versions are available on the system:
https://github.com/bytedeco/javacpp-presets/tree/master/numpy

@farizrahman4u

This comment has been minimized.

Copy link
Author

farizrahman4u commented Dec 1, 2019

@saudet no, we patch javacpp's numpy to support multi interpreter (limited support). The patch was written targeting an older version of numpy. Now it supports 1.7 too.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Dec 1, 2019

}
}

private static void applyPatches() {

This comment has been minimized.

Copy link
@farizrahman4u

farizrahman4u Dec 1, 2019

Author

@saudet We patch numpy here.

evalString("__random_path")
});

return patches;

This comment has been minimized.

Copy link
@farizrahman4u

farizrahman4u Dec 1, 2019

Author

@saudet There are the patches

int(os.environ.get('NUMPY_EXPERIMENTAL_ARRAY_FUNCTION', 0)))


ARRAY_FUNCTION_ENABLED = ENABLE_ARRAY_FUNCTION # backward compat

This comment has been minimized.

Copy link
@farizrahman4u

farizrahman4u Dec 1, 2019

Author

@saudet This var was renamed in numpy 1.7, so we alias it.

@@ -294,6 +294,8 @@

<python.version>3.7.5</python.version>
<cpython-platform.version>${python.version}-${javacpp-presets.version}</cpython-platform.version>
<numpy.version>1.17.3</numpy.version>

This comment has been minimized.

Copy link
@saudet

saudet Dec 1, 2019

Member

Ah, you're actually adding that to this PR, that's why I missed it. Looks good

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Dec 2, 2019

I can confirm this is running OK for me now on Windows, no more cache corruption issue.

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Dec 2, 2019

@farizrahman4u are we good to go to merge this?

@farizrahman4u

This comment has been minimized.

Copy link
Author

farizrahman4u commented Dec 2, 2019

yes :shipit:

@AlexDBlack AlexDBlack merged commit 1adc259 into master Dec 2, 2019
1 check was pending
1 check was pending
continuous-integration/jenkins/pr-head This commit is being built
Details
@AlexDBlack AlexDBlack deleted the fr_py_updates branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.