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

Fixing byte-order consistency/mismatch for cross-endian platform #1181

Merged
merged 12 commits into from Jun 11, 2021

Conversation

pradghos
Copy link
Contributor

@pradghos pradghos commented May 5, 2021

@pradghos pradghos changed the title Fixing byte-order consistency/missmatch for cross-endian platform Fixing byte-order consistency/mismatch for cross-endian platform May 5, 2021
@pradghos
Copy link
Contributor Author

pradghos commented May 5, 2021

Circle CI failed at ./continuous_integration/build_doc.sh

result = self._call_chain(self.handle_open, protocol, protocol +
File "/home/circleci/miniconda/envs/testenv/lib/python3.9/urllib/request.py", line 494, in _call_chain
  result = func(*args)
File "/home/circleci/miniconda/envs/testenv/lib/python3.9/urllib/request.py", line 1389, in https_open
  return self.do_open(http.client.HTTPSConnection, req,
File "/home/circleci/miniconda/envs/testenv/lib/python3.9/urllib/request.py", line 1349, in do_open
  raise URLError(err)
urllib.error.URLError: <urlopen error [Errno 110] Connection timed out>

Makefile:11: recipe for target 'html' failed

Before this commit https://github.com/joblib/joblib/pull/1181/commits/d9e5adbcc5478159310ebd58b3c070c5feea7e7d all were green except ci/circleci: build

Now joblib.joblib (Testing linux_py37_distributed) failed at test_nested_loop_error_in_grandchild_resource_tracker_silent which looks like unrelated to this PR.

Any pointers would really help . Thank you !

@pradghos
Copy link
Contributor Author

@ogrisel It would be great if you can take a look. Thanks in advance !

@lesteve
Copy link
Member

lesteve commented May 27, 2021

Thanks a lot for this PR! What would be great is to have a test for this.

Is there a way to store a pickle string obtained from a big endian machine and test that it can be loaded fine in the CI (little endian machine)?

I think we can ignore the CI failures as they do seem spurious.

@ogrisel
Copy link
Contributor

ogrisel commented May 27, 2021

Alternatively could you just please add a couple of unit tests for _is_numpy_array_byte_order_mismatch alone (independently of jolib.load / dump)?

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #1181 (f8a2f16) into master (754433f) will increase coverage by 0.02%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
+ Coverage   94.51%   94.54%   +0.02%     
==========================================
  Files          47       47              
  Lines        6997     7034      +37     
==========================================
+ Hits         6613     6650      +37     
  Misses        384      384              
Impacted Files Coverage Δ
joblib/test/test_numpy_pickle.py 93.33% <92.59%> (-0.04%) ⬇️
joblib/numpy_pickle.py 98.01% <100.00%> (+<0.01%) ⬆️
joblib/numpy_pickle_compat.py 91.08% <100.00%> (+0.18%) ⬆️
joblib/numpy_pickle_utils.py 93.25% <100.00%> (+0.57%) ⬆️
joblib/test/test_store_backends.py 97.14% <0.00%> (+5.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 754433f...f8a2f16. Read the comment docs.

@pradghos
Copy link
Contributor Author

pradghos commented Jun 8, 2021

@lesteve @ogrisel Thank you very much for the review. I have added test. Please let me know in case of any further comments.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Some nitpick. I plan to push a new commit to better test the byteswap operation itself after that.

joblib/test/test_numpy_pickle.py Outdated Show resolved Hide resolved
joblib/test/test_numpy_pickle.py Outdated Show resolved Hide resolved
@ogrisel ogrisel merged commit 0fa2cb9 into joblib:master Jun 11, 2021
@ogrisel
Copy link
Contributor

ogrisel commented Jun 11, 2021

Merged!, thanks @pradghos!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 7, 2021
Release 1.1.0
Fix byte order inconsistency issue during deserialization using joblib.load in cross-endian environment: the numpy arrays are now always loaded to use the system byte order, independently of the byte order of the system that serialized the pickle. joblib/joblib#1181
Fix joblib.Memory bug with the ignore parameter when the cached function is a decorated function. joblib/joblib#1165
Fix joblib.Memory to properly handle caching for functions defined interactively in a IPython session or in Jupyter notebook cell. joblib/joblib#1214
Update vendored loky (from version 2.9 to 3.0) and cloudpickle (from version 1.6 to 2.0) joblib/joblib#1218
jjerphan pushed a commit to jjerphan/joblib that referenced this pull request Oct 11, 2021
…lib#1181)



Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 14, 2021
Release 1.1.0
Fix byte order inconsistency issue during deserialization using joblib.load in cross-endian environment: the numpy arrays are now always loaded to use the system byte order, independently of the byte order of the system that serialized the pickle. joblib/joblib#1181
Fix joblib.Memory bug with the ignore parameter when the cached function is a decorated function. joblib/joblib#1165
Fix joblib.Memory to properly handle caching for functions defined interactively in a IPython session or in Jupyter notebook cell. joblib/joblib#1214
Update vendored loky (from version 2.9 to 3.0) and cloudpickle (from version 1.6 to 2.0) joblib/joblib#1218
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.

None yet

3 participants