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

explicitly np.load(..., allow_pickle=True) since we're building objects #1607

Merged
merged 1 commit into from Apr 23, 2019

Conversation

Projects
None yet
5 participants
@loriab
Copy link
Member

commented Apr 22, 2019

Need to explicitly allow pickles (which we need to since building objects) in NumPy 16.3. See numpy/numpy#12759 . @dgasmith, needed here, too? quicktests passes w/o. This should fix travis.

Checklist

  • Tests added for any new features
  • quicktests passes

Status

  • Ready for review
  • Ready for merge

@loriab loriab added this to the Psi4 1.4 milestone Apr 23, 2019

@PeterKraus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

The way I read the _np_read() function, that really is just numpy arrays (not classes or etc) - not a pickle file; perhaps try adding allow_pickle=False and see what breaks?

@JonathonMisiewicz

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

The way I read the _np_read() function, that really is just numpy arrays (not classes or etc) - not a pickle file; perhaps try adding allow_pickle=False and see what breaks?

allow_pickle=True was the NumPy default behavior, so this PR just maintains that for Psi. allow_pickle=False is the new default behavior, which breaks Psi. See this Travis to log to see the test cases that fail: https://travis-ci.org/psi4/psi4/jobs/523119052

@PeterKraus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

The way I read the _np_read() function, that really is just numpy arrays (not classes or etc) - not a pickle file; perhaps try adding allow_pickle=False and see what breaks?

allow_pickle=True was the NumPy default behavior, so this PR just maintains that for Psi. allow_pickle=False is the new default behavior, which breaks Psi. See this Travis to log to see the test cases that fail: https://travis-ci.org/psi4/psi4/jobs/523119052

Yes, but that's crashing because of _core_wavefunction_from_file(), not _np_read(), right? Raw log shows that 14 tests fail because of the former, and none because of the latter.

@loriab

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Yes, I think you're both right. the numpy_helpers np.load is serializing ordinary floats for Matrix/Vector, so np machinery, not pickle, used. Then the python_helpers np.load is forming the custom Matrix/Vector objects themselves, so they do need pickle True, and >=16.3 need it explicitly.

@PeterKraus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

I think the np-array-interface test should be checking for this already: it is hitting Matrix.np_read() after writing it into file.

@JonathonMisiewicz

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

I'm having a hard time following all this, so let me try to make sure I'm understanding your concerns about driver/p4util/numpy_helper.py:_np_read. I think you're suggesting we explicitly mark allow_pickle=False for that function because

  1. security reasons mean allow_pickle should be False unless needed to be True
  2. allow_pickle is not needed to be True for that function (as demonstrated by np-array-interface passing, even with Travis passing in False)
  3. some users may compile with pre-16.3 NumPy, where default definitions would make allow_pickle=True, which is bad by (1) and (2)

Did I get that right?

@robertodr robertodr merged commit daa59e9 into psi4:master Apr 23, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
psi4.psi4 #20190422.4 succeeded
Details

@loriab loriab deleted the loriab:npload branch Apr 23, 2019

@loriab loriab referenced this pull request May 13, 2019

Open

Psi4 v1.4 Release Notes #1562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.